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

working branch for supporting windows contributors #6313

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

davidcoleman007
Copy link

@davidcoleman007 davidcoleman007 commented May 2, 2024

As a windows developer, I am unable to install RSP and contribute.

Steps to reproduce:

  1. use windows
  2. clone this repo
  3. yarn it
  4. observe the failure seen below.

I have found a number of issues, mostly around the build-icons step.
image

after much trial and error (mostly error), i have found that the culprit is that we are specifying --extensions '.cjs,.js' when we build each of the icon bundles. for some reason, babel-node does not see this as a valid extension, and will actually simply ignore *.js anyway. Furthermore, this invalid extension value causes all other cli arguments to be ignored, and thus presets @babel/env is also ignored. this is where we get the cjs import transform from. Causing the above errors.

after that i had an error starting story book, due to improper path separators. The launch failed in git bash. This is because in git bash we need forward slashes, not back slashes. path is not aware of this nuance and incorrectly gives us backslashes on every shell that detects as win32. I have used the package is-mingw to solve this in our preview.js.

summarization of steps taken:

  • remove babel-node --extensions '.cjs,.js' - babel automatically will use .cjs, and for some reason this was causing babel to ignore all other cmd line args, including --presets @babel/env, which is where we get the cjs import transform on mac/linux
  • fix storybook preview paths for MinGW (git bash)

Closes (#6314)

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
    • will add details above
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

  1. use windows
  2. clone this repo and use my branch
  3. yarn it
  4. observe that it works
  5. yarn start
  6. observe that storybook starts and loads

🧢 Your Project:

add is-mingw
use isMinGW to use proper slashes on windows git bash
@@ -140,6 +140,7 @@
"full-icu": "^1.3.0",
"identity-obj-proxy": "^3.0.0",
"ignore-styles": "^5.0.1",
"is-mingw": "^2.0.0",
Copy link
Author

Choose a reason for hiding this comment

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

i don't need this.

i can do this instead:
const isMinGW = process.env.MSYSTEM === 'MINGW64' || process.env.MSYSTEM === 'MINGW32';

});
let szConfigImports = configImports.join('\n');
if (isMingw()) {
storiesPath = storiesPath.replace(/\\/g, '/');
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than doing .replace, we could do something more like...

> path.posix.join('a','b')
'a/b'
> path.win32.join('a','b')
'a\\b'

so

let curPath = path;
if (isMingw()) {
  curPath = path.posix;
}

and then all the "join"/"relative" things would work

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.

None yet

2 participants