INSTUI-3823 Get InstUI build on Windows #1241
Conversation
also remove concurrently dependency. This simplifies our script execution a lot. Some scripts are a bit more colorful, some output a bit more log
| let result = { status: 1 } | ||
|
|
||
| try { | ||
| result = runCommandSync(`${resolveBin('concurrently')}`, args) |
There was a problem hiding this comment.
This was the biggest issue. This is an ugly hack to allow synchronous parallel execution in node (= execute multiple scripts simultaneously synchronously). This was accomplished by running concurrently (a lib to execute multiple scripts at the same time) synchronously. This had multiple issues:
- we needed to pass arguments and env variables in a huge string like
concurrently "ENV_VAR1=A, ENV_VAR2=B scriptA --arg1 abc --arg2 bcd" ENV_VAR3=3 scriptB --arg1=123that is very hard to read and is not Windows compatible. - We were running
concurrentlynot with native code but with a lib calledexeca. Dontknow why - All these scripts executing scripts made IDE debuggers get lost unable to place breakpoints
- some of them also mess with
stdoutcausing formatting like colors to be lost.
This solution is pure native code, easy to read, crossplatform with a price that runCommandsConcurrently became async (we never made use of its synchronous nature)
matyasf
changed the title
|
Preview URL: https://1241--preview-instui.netlify.app |
| @@ -36,7 +36,7 @@ | |||
| "generate:component": "yarn workspace @instructure/instui-cli instui create component", | |||
| "generate:package": "yarn workspace @instructure/instui-cli instui create package", | |||
| "commit": "ui-scripts commit", | |||
| "bootstrap": "time yarn node scripts/bootstrap.js", | |||
There was a problem hiding this comment.
time does not exist on Windows, we should use a node API in a later PR
|
It seems that |
react-docgen 6 upgrade required that the docs build scripts use ESM imports. It also has a bug with some function definitions (see react-docgen issue 827 on Github) that required the rewrite of some types. Also made doc build scripts a bit more Windows friendly
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -22,7 +22,7 @@ | |||
| * SOFTWARE. | |||
| */ | |||
|
|
|||
| import type { LibraryOptions, ParsedDoc, ProcessedFile } from '../DataTypes' | |||
| import type { LibraryOptions, ParsedDoc, ProcessedFile } from '../DataTypes.mjs' | |||
There was a problem hiding this comment.
This rewrite was needed because I thought that an issue would be fixed by upgrading react-docgen... it was not, and it required that our TS files are emitted with ESM imports. This needed a change in the extension otherwise they'd be treated as commonJS files.
| if (process.platform === 'win32' && module.description) { | ||
| // JSDoc bug https://github.com/jsdoc/jsdoc/issues/2067 | ||
| module.description = module.description.replace(/\r/g, "\r\n") |
There was a problem hiding this comment.
ugly hack that is the actual fix
| onUpdate?: ({ visibleItemsCount }: { visibleItemsCount: number }) => void | ||
| onUpdate?: (visibleItemsCount: { visibleItemsCount: number }) => void |
There was a problem hiding this comment.
this was needed because react-docgen was failing with the old code, see reactjs/react-docgen#827
matyasf
changed the title
This PR gets
yarn bootstrapandyarn devworking on windows.yarn testalso runs, but its quite flaky -- some tests fail, some others just throw an error.This is accomplished by using
cross-spawnvia its API to spawnbabel,karmaandwebpackinstead ofconcurrentlyandexecavia command line.On OSX nothing should change except
babelbeing a bit more verbose.To test:
On OSX run our usual build/test commands, especially with parameters (e.g.
yarn test --scope..).On Windows run
bootstrap,dev. If you get tons of popups change the default program to runjsfiles tonode.exe