build: Swapping NPM for PNPM Step 2 of 5#3040
build: Swapping NPM for PNPM Step 2 of 5#3040scotttjob wants to merge 2 commits intoscott_t/pnpm-upgrade-chain-1from
Conversation
| "require": "./dist/index.cjs" | ||
| } | ||
| }, | ||
| "devDependencies": { |
There was a problem hiding this comment.
formatters was building only because Rollup plugins were available via hoisting. pnpm requires them to be declared where they’re used.
| "dist/*" | ||
| ], | ||
| "devDependencies": { | ||
| "@apollo/react-testing": "^4.0.0", |
There was a problem hiding this comment.
hooks had the same problem as formatters: Rollup plugins and Apollo test utilities were being resolved transitively. pnpm surfaces that as missing direct deps.
| ], | ||
| "dependencies": { | ||
| "@floating-ui/react": "^0.27.5", | ||
| "@floating-ui/utils": "0.2.11", |
There was a problem hiding this comment.
components was relying on transitive/workspace deps like @jobber/hooks, @apollo/client, @floating-ui/utils, and tslib. pnpm requires those to be explicit because package boundaries are enforced more strictly.
There was a problem hiding this comment.
Explicit dependencies 😍
| }, | ||
| "devDependencies": { | ||
| "@gorhom/bottom-sheet": "^5.2.8", | ||
| "@jobber/hooks": "*", |
There was a problem hiding this comment.
components-native uses @jobber/hooks but wasn’t declaring it itself. pnpm exposed that hidden workspace dependency, so we made it explicit in both dev/peer declarations.
| @@ -21,6 +21,7 @@ | |||
| "visual:test:single": "node scripts/visualTestHelper.mjs test" | |||
| }, | |||
| "dependencies": { | |||
There was a problem hiding this comment.
site was also picking up several runtime/build deps transitively from other packages or the repo root. pnpm makes those missing declarations fail in isolation.
| import multiInput from "rollup-plugin-multi-input"; | ||
| import nodePolyfills from "rollup-plugin-polyfill-node"; | ||
|
|
||
| const createMultiInput = |
There was a problem hiding this comment.
The Rollup multi-input plugin shape is less forgiving once each package resolves its own installed copy. This change makes the config work with the module export shape pnpm exposes.
| @@ -2,15 +2,14 @@ | |||
| module.exports = { | |||
There was a problem hiding this comment.
pnpm’s layout makes deep ../../node_modules/... assumptions brittle. require.resolve(...), explicit roots, and looser transform ignores make Jest resolve packages correctly without hoisting assumptions.
| import { DocumentNode } from "@apollo/client"; | ||
| import { MockedProvider, MockedResponse } from "@apollo/react-testing"; | ||
| // eslint-disable-next-line import/no-internal-modules | ||
| import { MockedProvider } from "@apollo/client/testing"; |
There was a problem hiding this comment.
@apollo/react-testing was the old/indirect path. With pnpm’s stricter graph, we now import Apollo testing helpers from the package that is actually declared and used.
| cursor: pageInfo.endCursor, | ||
| }, | ||
| updateQuery: (prev, { fetchMoreResult }) => | ||
| fetchMoreUpdateQueryHandler(prev, fetchMoreResult, getCollectionByPath), |
There was a problem hiding this comment.
Once hooks typechecks against its own resolved Apollo/TS versions, generic inference is stricter. These casts are there to keep fetchMore/subscribeToMore update callbacks type-safe under the newer isolated compile.
| const elements = [ | ||
| ...ref.querySelectorAll<HTMLElement>(focusables.join(", ")), | ||
| ]; | ||
| const elements = Array.from( |
There was a problem hiding this comment.
Minor DOM typing cleanup surfaced by stricter package-local TS checking.
| readonly [index: string]: boolean | string | undefined; | ||
| } | ||
|
|
||
| type KeyComparator = XOR<VerboseKeyComparator, SimpleKeyComparator>; |
There was a problem hiding this comment.
ts-xor was only needed to express a stricter utility type, but under pnpm isolation that dependency path becomes unnecessary friction. Replacing it with a plain union removes an avoidable type-only coupling.
There was a problem hiding this comment.
I really like this especially because I have noticed that XOR is really silly when dealing with array types which useOnKeyDown uses in its parameter keys: KeyComparator[] | KeyComparator
| startedInsideRef?: MutableRefObject<boolean> | undefined; | ||
| } | ||
|
|
||
| export interface UseModalProps { |
There was a problem hiding this comment.
Explicit hook return types stabilize typechecking once the package is compiled in isolation against its own resolved React/TS types, instead of inheriting broader hoisted inference.
| direction: KeyboardAction, | ||
| ) => void; | ||
| }) { | ||
| useOnKeyDown((event: KeyboardEvent) => { |
There was a problem hiding this comment.
Small callback typing cleanup needed after the useOnKeyDown type simplification above.
| } from "../../DataList.types"; | ||
|
|
||
| const defaultLayout = (children: object) => ( | ||
| <>{children as unknown as React.ReactNode}</> |
There was a problem hiding this comment.
React’s fragment child typing became stricter under the currently resolved types, and pnpm exposed that by removing ambient type leakage. The cast keeps the existing fallback behavior while making the default context value typecheck.
I tried to keep this without a horrible cast, but it tried to unwind half of DataList and I'd rather not dive into a deprecated component.
Deploying atlantis with
|
| Latest commit: |
24a603d
|
| Status: | 🚫 Build failed. |
2 participants
Motivations
Changes
I've attached codex comments to code directly, for why certain things are changing.
Testing
The application should still work as-is.
Changes can be
tested via Pre-release
In Atlantis we use Github's built in pull request reviews.