chore(ide-extension): upgrade project setup #347
Conversation
openscript
commented
Feb 3, 2023
•
openscript
commented
- Adds a dev container configuration
- Updates contributing.md according to the dev container configuration
- Updates dependencies
- Enhances project configuration
- Enhances launch scripts
- Removes Rollup (VSCode extension template removed it too. An option is to use webpack, but it makes the project setup complicated and I don't think inlang needs it)
There was a problem hiding this comment.
I propose to not merge this PR due to:
- The PR introduces changes that are unrelated to the container setup (and might be reverted by the follow up PR).
- The container setup does not work for the codebase except for the ide extension.
- Merging this PR will likely lead to merge conflicts. Numerous changes are required that will lead to merge conflicts with the follow-up PR that re-introduces the IDE extension. I propose waiting for the other PR first.
Regarding 2., the container setup only works for the ide extension. Turborepo, and therefore the entire code base, fails to run. I suspect the "node-typescript" dev container is to blame because it only includes "node"? I take that #312 is a larger undertaking but dev containers seem to be the way to go and I should set them up!
Feel free to close this PR if you agree that we should wait for the follow-up PR and introduce a working container setup separately.
| /** | ||
| * What is rollup used for? | ||
| * | ||
| * Inlang packages are ES Modules. VSCode only supports CJS (CommonJS modules). | ||
| * Rollup is used to compile the ESM code to CJS. | ||
| */ | ||
|
|
There was a problem hiding this comment.
Unrelated to this PR but we likely need rollup to bundle ESM to CommonJS (you mentioned you had issues with ESM on Discord).
There was a problem hiding this comment.
I don't completely understand if VSCode extensions really need to be CJS modules.
At the other hand officially VS Code doesn't use Rollup anymore. They removed it from their templates and tutorials.
What I try here is to stick to their proposed set up.
There was a problem hiding this comment.
If it works great, one dependency less. Electron does not support ESM though. I suspect the official template to use CJS.
|
|
||
| try { | ||
| // Run the mocha test | ||
| mocha.run(failures => { |
There was a problem hiding this comment.
Also unrelated to this PR
Would using vitest for tests be possible?
The rest of the codebase uses vitest. Mainly because of out of the box support for ESM and TypeScript.
There was a problem hiding this comment.
Yes, I think this is possible.
| // Not specifying node16 causes https://github.com/rollup/plugins/issues/1224#issuecomment-1242109375 | ||
| // interestingly, extending a base config taht contains module node16 still requires explicit manual | ||
| // setting of module node16 here. | ||
| "module": "Node16", |
There was a problem hiding this comment.
Unrelated to this PR
Can we use Node16 as the module resolution or the ide extension?
Using Node16 as the module resolution algorithm ensures compatibility with the rest of inlang's codebase and is the resolution standard that (finally) the JS ecosystem agreed on.
| }, | ||
| "scripts": { | ||
| "vscode:prepublish": "npm run compile", | ||
| "compile": "tsc -p ./", |
There was a problem hiding this comment.
Unrelated to this PR
Please use build for compiling a production build to align with the rest of the source code.
| 1. (If running, stop `npm run dev`.) | ||
| 2. Press `F5`. | ||
| 3. Wait until the terminal is showing "listening on localhost:3000". | ||
| 4. Open "localhost:xxxx" in the browser that has been opened. |
There was a problem hiding this comment.
In theory, pressing F5 started the entire codebase in debug mode without any setup. I suppose that F5 did not work?
There was a problem hiding this comment.
I'll add another paragraph for this and mention the launch configuration.
|
I could use turbo and successfully build the entire project. What errors you get? |
Do you want to continue with this PR? Otherwise, I'll open another one that only introduces dev containers. |
|
No problem for me if you open another one and I'm also happy to contribute there. |
Taken from #347. Using javascript node 18 because typescript is installed with NPM. The image is thus smaller + has one dependency less.
|
Closed by #363. |
