Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Use POSIX paths when inserting import statements correctly this … #797

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

nmn
Copy link
Contributor

@nmn nmn commented Dec 5, 2024

Fixes #774

I was finally able to test this fix on a Windows machine and this finally works as expected.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 5, 2024
Copy link

github-actions bot commented Dec 5, 2024

workflow: benchmarks/size

Comparison of minified (terser) and compressed (brotli) size results, measured in bytes. Smaller is better.

@stylexjs/[email protected] size:compare
./size-compare.js /tmp/tmp.I5hhWV9PbE /tmp/tmp.y5g6aDRi0l

Results Base Patch Ratio
stylex/lib/stylex.js
· compressed 729 729 1.00
· minified 2,541 2,541 1.00
stylex/lib/StyleXSheet.js
· compressed 1,266 1,266 1.00
· minified 3,776 3,776 1.00
rollup-example/.build/bundle.js
· compressed 562,983 562,983 1.00
· minified 10,185,368 10,185,368 1.00
rollup-example/.build/stylex.css
· compressed 99,229 99,229 1.00
· minified 745,845 745,845 1.00

@nmn nmn merged commit 3e5a229 into main Dec 5, 2024
7 of 8 checks passed
@nmn nmn deleted the fix/windows-import-path-2 branch December 5, 2024 12:06
@BjornHMS
Copy link

BjornHMS commented Dec 5, 2024

Hey @nmn thanks for all your work on this.
This doesn't seem to work either, I now get:

import "../C:/projects/website/src/stylex_bundle.css";

As you can see it's not actually making a relative path.
I think path.posix.relative expects the format to not be in a windows absolute path format.

However path.relative was returning a relative path, but the slashes were wrong... so I changed path.posix.relative back to path.relative and used the new toPosixPath function .. and now it works!

Edit: to clarify .. The only changes needed for this branch to work for me was to change path.posix.relative to path.relative.

@nmn
Copy link
Contributor Author

nmn commented Dec 5, 2024

@BjornHMS Yes, you're right! That was the code I tested on my Windows test machine, but there was an oversight when I rebased my PR. I'll fix it.

@nmn
Copy link
Contributor Author

nmn commented Dec 5, 2024

Created #801

aminaopio pushed a commit to aminaopio/stylex that referenced this pull request Dec 22, 2024
aminaopio pushed a commit to aminaopio/stylex that referenced this pull request Dec 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getting errors with using the stylex CLI in next 15 App dir.
3 participants