fix(babel): normalize resolved paths to POSIX so Windows rewrites work - #390
Open
YevheniiKotyrlo wants to merge 4 commits into
Open
fix(babel): normalize resolved paths to POSIX so Windows rewrites work#390YevheniiKotyrlo wants to merge 4 commits into
YevheniiKotyrlo wants to merge 4 commits into
Conversation
The import plugin's relative-import handlers resolve a source against the file being transformed and then match the result against forward-slash literals (`react-native/Libraries/Components/`, `react-native-web/dist`). path.resolve returns backslash-separated paths on Windows, so those splits never match and the import is left un-rewritten. Only relative imports break, and only on Windows -- bare specifiers are plain string matches; the failing cases were green on the Linux/macOS CI. Add a `resolvePosix` helper (resolve + normalize \ -> /) and use it in parseReactNativeSource / parseReactNativeWebSource. Resolve semantics are unchanged; only the separator is normalized. import-plugin's isFromThisModule stays on path.resolve -- it compares two OS-native paths, so it already works. Adds a helpers unit test asserting the POSIX invariant.
`resolvePosix` fused `path.resolve` into the helper, so every test of it had to go through `resolve` — and on Linux `resolve` never emits a backslash. The normalization had nothing to act on there, so the tests passed with it deleted. CI runs ubuntu-latest and nothing else, which is how the three failing cases sat in main unnoticed. `toPosixPath` is the primitive now and `resolvePosix` composes with it, so a test can feed it a Windows-shaped literal and fail on any host. Deleting the normalization turns 8 tests red across 3 suites, three of them host-independent. Left unconditional rather than gated on `sep`. Gating is tempting — a POSIX filename may legally contain a backslash — but it makes the function an identity on Linux and puts the guard back out of CI's reach. The hazard it would close needs a directory literally named `react-native\Libraries\Components\` on a POSIX host, and the failure mode is a missed rewrite rather than wrong output.
A backslash is a legal filename character on POSIX, so rewriting one there corrupts a path that was already correct. The gate belongs at the boundary where a host path enters — resolvePosix — rather than inside toPosixPath, which stays a pure transform so a test can feed it a Windows-shaped literal and observe the result on any host.
…ards
The Windows separator fix stands: `resolveImportSource` normalizes
`path.resolve`'s output to POSIX before the handlers match it against
forward-slash literals. The host separator is now an argument to
`toPosixPath` rather than a module-level `sep` read, so both branches are
reachable from any host — CI runs only ubuntu-latest and one macos-15, and
an assertion driven through `path.resolve` is vacuous there.
Auditing that path turned up three more defects in the same few lines.
`react-native-web.ts` resolved a relative source against the filename where
it meant the filename's directory, consuming one `..` too few and moving the
package boundary by a directory; `react-native.ts` already used `dirname`.
Both now call one helper whose signature gives the caller no base to get
wrong.
`processed.has(path)` could never be true: only `Statement` nodes are added
to that set and a `NodePath` is not one. Throw-injecting it leaves the whole
suite green, while the same probe on `path.node` trips on nearly every
rewrite — that sibling is the live re-entry guard. The dead disjunct is gone
and the set is typed `WeakSet<Statement>`, so re-adding it is a compile
error.
`isFromThisModule` derived the package root as `../../../` from `__dirname`,
which names it in the built layout and points outside the package when the
plugin runs from `src/`. It also read `.startsWith` off `state.filename`,
which babel types `string | undefined` and leaves undefined when a caller
passes none — that threw before any rewrite was considered. The root now
comes from the nearest `package.json` declaring a `name` (builder-bob writes
a bare `{ "type": ... }` manifest into each output directory), each shipped
directory is compared with a trailing separator, and the filename is
narrowed once per visitor.
That guard being live is why three existing suites change: babel-plugin-tester
infers `filepath` from the test file's own path, and a file under
`src/__tests__/` genuinely is one of this package's sources. Their
`babelOptions.filename` never reached babel at all. Each now sets `filepath`
to an application path, which is what those cases always meant.
Tests: the plugin end-to-end through `transformSync`, this package's own
sources in both layouts, the package-boundary cases, and the first coverage
of the metro resolver plane — pinned against the babel plane over the census
they share, since the two are alternatives selected by
`globalClassNamePolyfill` and must agree.
`plugin.test.mts` is deleted. Jest 29 collects neither the `.mts` extension
nor that testMatch shape, its first case carries `only: true`, and that case
expects output the plugin does not emit. The two shapes no collected suite
covered are ported over with the expectations the plugin actually produces.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The babel import plugin's relative-import rewriting silently no-ops on Windows.
parseReactNativeSource/parseReactNativeWebSourceresolve a relative source against the file being transformed and then match the result against forward-slash literals:path.resolvereturns backslash-separated paths on Windows, so.split("react-native/Libraries/Components/")(and thereact-native-web/distsplit) never matches —internalPathisundefined, the function bails, and the import is left un-rewritten. The plugin's other cases (barereact-native/react-native-webspecifiers) are plain string matches and are unaffected, so only relative imports break, and only on Windows.This surfaces as three tests failing on Windows while green on the Linux/macOS CI:
src/__tests__/babel/react-native.test.ts›7. import View from '../View/View';src/__tests__/babel/react-native-web.test.ts›6. import View from '../View';src/__tests__/babel/react-native-web.test.ts›17. const View = _interopRequireDefault(require('../View'));Fix
Normalize the resolved path to POSIX separators before the forward-slash matching, via a small shared helper:
parseReactNativeSourceandparseReactNativeWebSourcenow callresolvePosixinstead ofresolve. The resolve semantics are unchanged — each keeps its own base (dirname(filename)vsfilename); only the separator is normalized.import-plugin.ts'sisFromThisModuleis intentionally left onpath.resolve: it compares two OS-native absolute paths (state.filename.startsWith(thisModuleDist)), so both sides share the platform separator and it already works — normalizing only one side would break it.Test plan
src/__tests__/babel/goes from 2 suites failing / 3 tests to 4 suites / 35 tests.src/__tests__/babel/helpers.test.tsasserts against Windows-shaped literals, so it fails on any host when the normalization is removed. DeletingtoPosixPath's body turns 8 tests red across 3 suites.yarn test,yarn typecheck,yarn eslintandprettier --checkall pass.Why the helper is split in two
An earlier version of this exposed a single
resolvePosix(...segments)that fusedpath.resolveinto the normalization. That is untestable on the platform that matters:resolveonly emits a backslash on Windows, so on Linux the normalization has nothing to act on and an assertion throughresolvePosixpasses with the fix deleted. I measured that — emulating the CI runner, the fused helper's tests stay green under every mutation, including removing the fix entirely.Since every job in
ci.ymlisubuntu-latest(with onemacos-15for the iOS build) and there is no Windows runner anywhere, a guard that only fails on Windows guards nothing here.toPosixPathis a pure string transform, so it can be fed"C:\\…"directly and fails everywhere.The platform gate sits in
resolvePosix, not intoPosixPath. A backslash is a legal filename character on POSIX, so rewriting one there would corrupt a path that was already correct — but putting that check insidetoPosixPathwould make it an identity on Linux and put it back out of CI's reach. Splitting them gives both: the pure transform is testable on any host, and the rewrite only ever runs wherepath.resolvecan actually produce a backslash.Worth considering separately: adding
windows-latestto thetestjob would make this class of bug visible at all. Without it, this fix is guarded only by tests the project's own runner cannot fail. Happy to send that as its own PR.Two adjacent things I did not touch
src/metro/resolver.tssolves the same "find a package inside a host path" problem the other way, building its markers fromsep. POSIX normalization is right here specifically, because the resolved path is re-emitted as a module specifier atreact-native.ts:26and a specifier must use forward slashes — asepmarker would leaveView\Viewin the rebuilt string. But the package now has two conventions for one problem, and that is worth a decision.import-plugin.ts:29-36'sisFromThisModuleguard is dead on every OS, and not for separator reasons:thisModuleDistis absolute while Metro passesstate.filenameproject-relative (Transformer.js:68doespath.relative(projectRoot, filePath)), so thestartsWithcan never match. Deliberately out of scope here — flagging it because it sits three lines from this change.