Skip to content

fix(babel): emit forward slashes in generated import paths on Windows - #5054

Open
KallinikosMil wants to merge 2 commits into
callstack:mainfrom
KallinikosMil:fix/mappings-windows-path-separator
Open

fix(babel): emit forward slashes in generated import paths on Windows#5054
KallinikosMil wants to merge 2 commits into
callstack:mainfrom
KallinikosMil:fix/mappings-windows-path-separator

Conversation

@KallinikosMil

Copy link
Copy Markdown

Motivation

scripts/generate-mappings.ts builds each mapping's path with path.relative, which returns platform-native separators. On Windows that writes backslashes into lib/mappings.json:

"Palette": { "path": "lib\module\theme\tokens", "name": "Palette" }

The babel plugin then interpolates that value straight into an import specifier (`${name}/${mapping.path}` in src/babel/index.js), so the rewritten imports come out as:

import { Palette } from "react-native-paper/lib\module\theme\tokens";

Import specifiers always use forward slashes regardless of platform, so these paths don't resolve — the plugin's output is broken for anyone generating the mappings on Windows.

This normalises the separator to / at the point where the path is produced. index is taken from package.json's main, which is already POSIX, so only the relative() result needed fixing.

Related issue

No existing issue — I ran into it while running the test suite on Windows, where src/babel/__tests__/index.js fails on main for this reason.

Test plan

yarn jest src/babel fails on main on Windows and passes with this change:

- import { Palette } from "react-native-paper/lib/module/theme/tokens";
+ import { Palette } from "react-native-paper/lib\module\theme\tokens";

The regenerated lib/mappings.json now contains "lib/module/theme/tokens" on Windows, matching what the existing __fixtures__/generate-mappings/output.js snapshot expects.

The change is a no-op on Linux/macOS, where path.sep is already /, so the existing CI result is unaffected. Full suite locally: 55 suites / 735 tests passing, lint and typecheck clean.

@KallinikosMil

Copy link
Copy Markdown
Author

Worth noting there's already precedent for this in the repo — scripts/generate-component-docs.ts normalises the same way when it emits a path into JSON:

return relativeDependency.split(path.sep).join('/');

So this brings generate-mappings.ts in line with how the other generator already handles it, rather than introducing a new convention.

(CI on this PR is sitting at action_required — it needs a maintainer to approve the workflow run, since this is my first PR here.)

@MikitasK MikitasK left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good! 👍
also, I suppose, it'd be great to add Windows CI job for yarn jest src/babel so this regression stays covered

@KallinikosMil

Copy link
Copy Markdown
Author

Thanks @MikitasK! Added in b015db38 — a Babel plugin (Windows) job on windows-latest running yarn jest src/babel. Without the fix in this PR it reproduces the failure, so the regression stays covered.

One thing worth flagging: the job sets core.autocrlf input before checkout. The repo has no .gitattributes, and Git for Windows defaults to core.autocrlf=true, so a plain checkout on a Windows runner gives CRLF working-tree files. That breaks toMatchFile against __fixtures__/generate-mappings/output.js (and makes yarn lint fail repo-wide with Delete ␍`` — I hit that locally before pinning it per-repo).

I kept it scoped to the new job so nothing else changes. If you'd rather fix it once for every Windows contributor, adding a .gitattributes with * text=auto eol=lf would let that step go away — happy to swap it in if you prefer that.

@MikitasK

Copy link
Copy Markdown

Thanks @MikitasK! Added in b015db38 — a Babel plugin (Windows) job on windows-latest running yarn jest src/babel. Without the fix in this PR it reproduces the failure, so the regression stays covered.

One thing worth flagging: the job sets core.autocrlf input before checkout. The repo has no .gitattributes, and Git for Windows defaults to core.autocrlf=true, so a plain checkout on a Windows runner gives CRLF working-tree files. That breaks toMatchFile against __fixtures__/generate-mappings/output.js (and makes yarn lint fail repo-wide with Delete ␍`` — I hit that locally before pinning it per-repo).

I kept it scoped to the new job so nothing else changes. If you'd rather fix it once for every Windows contributor, adding a .gitattributes with * text=auto eol=lf would let that step go away — happy to swap it in if you prefer that.

thanks @KallinikosMil 🙏
keeping line-ending configuration scoped to this job makes sense 👍 let’s leave .gitattributes for a separate change if we decide to enforce LF repository-wide

@KallinikosMil

Copy link
Copy Markdown
Author

Sounds good — leaving .gitattributes out of this one.

One mechanical thing before this can go in: the workflow runs on this PR are all sitting at action_required (first-time contributor to this repo), so CI has never actually executed — including the new Babel plugin (Windows) job. Whenever a maintainer clicks Approve and run on the run for b015db38 it should go green; I'd rather you see that job pass than take my word for it.

Happy to rebase if main has moved in the meantime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants