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

generator-langium: Transform generated project to npm workspace #1520

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

Conversation

kaisalmen
Copy link
Contributor

@kaisalmen kaisalmen commented May 29, 2024

TODO:

  • Align template structure with target structure
  • Integrate a global tsconfig.build.json in target project
  • Unify how build and compile is done in target project
  • Update to the latest version of the wrapper lib stack
  • Fix Build issues: Create index files and export code to other packages
  • Fix regular generator-langium unit tests
  • Add new test that check the complete structure
  • Option test not yet included even if selected
  • tsconfig.build.json needs to be dynamically created. Sources not contained should not be included as it leads to build failures otherwise.
  • Align dependencies and update vitest
  • Remove the need to use http-server

Fixes #1495

TODO: from review:

  • Update langium-quickstart.md
  • Add clean script and build:clean to all packages

@kaisalmen kaisalmen changed the title WIP: Transform generated project to npm workspace WIP: generator-langium: Transform generated project to npm workspace May 29, 2024
@kaisalmen
Copy link
Contributor Author

kaisalmen commented Jun 5, 2024

@msujew @Yokozuna59 the generated workspace project builds again. Now, I need to fix the open points above ⬆️

@kaisalmen kaisalmen force-pushed the kaisalmen/issue-1495 branch 4 times, most recently from ea748fa to 16211b6 Compare June 14, 2024 09:38
@kaisalmen kaisalmen marked this pull request as ready for review June 14, 2024 09:39
@kaisalmen
Copy link
Contributor Author

@msujew @Yokozuna59 this is now ready for review. The only thing left is from the list is removing the need for http-server in the web package, because vite preview should do the job. But this is not blocking the review.

@kaisalmen kaisalmen changed the title WIP: generator-langium: Transform generated project to npm workspace generator-langium: Transform generated project to npm workspace Jun 14, 2024
@Lotes
Copy link
Contributor

Lotes commented Jun 14, 2024

For me workspaces were hard to work with in the beginning, because I was switching to the packages folders to run npm run X. Would be good to have some documentation on our website, I think. Like workspace 101, especially with the -w flag to address packages.
Does it change anything for the existing documentation?

@kaisalmen
Copy link
Contributor Author

Does it change anything for the existing documentation?

Very likely 🙂 We need to update it once this is merged.

@kaisalmen
Copy link
Contributor Author

Like workspace 101

The generated project is the blueprint. 👍 It features all facets: TS with specific test configs, VSCode eslint compatibility (root tsconfig.json per package), workspace eslint check, global build and top-level scripts that directly execute package scripts.

@kaisalmen
Copy link
Contributor Author

All points are resolved. I also fix an issue with the web example. I broke the configuration during this update. It is working again.

Copy link
Contributor

@Yokozuna59 Yokozuna59 left a comment

Choose a reason for hiding this comment

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

I'm not sure if I should review this PR or not, but since it was mentioned, I reviewed it.

The below review is just comments or questions regarding this approach, which could be ignored if you don't feel they are correct.


One more thing, since you updated the out directory here:

I think you may need to update the .gitignore.txt:

packages/generator-langium/package.json Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this file need to modified since some files are moved to different directories.

I would recommend to move the file to the root directory then reference the files from there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, good point. I overlooked that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added a TODO for this in PR description. ⬆️

packages/generator-langium/templates/package.json Outdated Show resolved Hide resolved
packages/generator-langium/templates/package.json Outdated Show resolved Hide resolved
packages/generator-langium/templates/.vscode/launch.json Outdated Show resolved Hide resolved
packages/generator-langium/templates/package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@Yokozuna59 Yokozuna59 left a comment

Choose a reason for hiding this comment

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

@kaisalmen
Copy link
Contributor Author

@Yokozuna59 thank you for the review. I still need to implement the two bigger points (added to the description) and then I am done. Any further enhancements should go to a next PR, because this one is already quite large, but there was no way around.

@kaisalmen
Copy link
Contributor Author

@Yokozuna59 and @msujew all TODOs are done. We need to update tutorials and docs on the website as well. This change has some impact.

@kaisalmen
Copy link
Contributor Author

I rebased the branch after Langium 3.1.0 release to resolve the conflicts.

@kaisalmen
Copy link
Contributor Author

@msujew I just updated to the latest wrapper version. Do you have time to review it this week?

@@ -0,0 +1,45 @@
{
"name": "<%= extension-name %>-language",
Copy link
Member

Choose a reason for hiding this comment

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

Issue: This package name is built by using the specified extension-name. However, all references to this package (i.e. tests, vscode extension, etc.) use the specified language-id. If I call my extension abc and my language cba:

  1. The language package will be called abc-language.
  2. All imports for that language will attempt to import cba-language.

This obviously yields errors on build. Both should use the same ID.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@msujew then language-id is the better choice. extension-name is only useful for the VSCode extension. Do you agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have changed it like suggested for now. ⬇️

export * from './generated/ast.js';
export * from './generated/grammar.js';
export * from './generated/module.js';
export { default as monarchSyntax } from './syntaxes/<%= language-id %>.monarch.js';
Copy link
Member

Choose a reason for hiding this comment

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

Question: We should only conditionally generate this export - if the web option is selected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, true, I will adapt this.

Copy link
Contributor Author

@kaisalmen kaisalmen Jul 8, 2024

Choose a reason for hiding this comment

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

This was a bit more work, because I needed to change the workflow, but it looks good now, IMO.

@@ -11,5 +11,5 @@
"out": "src/syntaxes/<%= language-id %>.monarch.ts"
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: As mentioned above, we should try to make this conditional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, true, I will adapt this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, see above.

Comment on lines 7 to 8
"textMate": {
"out": "syntaxes/<%= language-id %>.tmLanguage.json"
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: The textmate grammar is generated into a folder in the language package. However, it is referenced the package.json of the VS Code extension as if it were located inside of the extension package. Unless the adopter changes the generated path or copies the file manually, they don't get syntax highlighting in VS Code.

Copy link
Contributor Author

@kaisalmen kaisalmen Jul 5, 2024

Choose a reason for hiding this comment

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

I added a build prepare step in both web and extension that copies the file over. ⬇️

@kaisalmen
Copy link
Contributor Author

@msujew rework is complete. Im have not closed some of the discussions, because you should check if you agree with my solutions.

@aabounegm
Copy link
Member

@kaisalmen I created a project from this branch and it seems there is an issue with the generated CLI. I think the following paths inside packages/generator-langium/templates/packages/cli should be updated:

  • In bin/cli.js, it should import from just '../out/main.js' (without /cli)
  • In src/main.ts:13, the packagePath should have one less '..'

At least that's what I edited manually in my project for it to work

@kaisalmen
Copy link
Contributor Author

@aabounegm Thank you very much! Many things changed, it is easy to overlook things. I will push an update.

@zeyad001
Copy link

any updates on this?
Will this be part of a 3.2 release?

Copy link
Member

@aabounegm aabounegm left a comment

Choose a reason for hiding this comment

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

I just noticed another issue while packaging my extension. Also, npm run lint from root is broken, the path passed to the command should be ./packages/*/src instead of just src.

@aabounegm
Copy link
Member

aabounegm commented Jul 25, 2024

And one more thing: it seems that vsce does not currently support npm workspaces (microsoft/vscode-vsce#777 (comment)), so running vsce package tries to bundle the entire project (with all workspaces) and fails with weird errors.
Until this is fixed, there are 3 possible workarounds:

  1. Add ../ and ../../ to .vscodeignore
  2. Add "vsce": { "dependencies": false } to package.json
  3. Add a note that users need to pass --no-dependencies to the vsce command

I personally opted for the first option in my project with a comment linking to the issue.

And on a somewhat related note (also publishing), I would suggest adding a .gitignore file to packages/extension with at least *.vsix.

@spoenemann
Copy link
Contributor

We used solution 2 in our own extension, too:

"vsce": {
"dependencies": false
},

Copy link
Member

@aabounegm aabounegm left a comment

Choose a reason for hiding this comment

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

I got this thought while exploring how a JetBrains plugin would look like, and realized I'm copying main.cjs from extension/out/language rather than from language/out directly.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason this file resides under the extension folder and not under language?

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume chase it’s specific to the use case as packages/generator-langium/templates/packages/web/src/main-browser.ts is

Copy link
Member

Choose a reason for hiding this comment

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

Except that this one runs a language server regardless of the context as long as Node is available. The browser is the exception because it does not have Node.js or access to the file system.
For example, I installed LSP4IJ in WebStorm and pointed it to this file (node /.../extension/out/language/main.cjs --stdio) and it ran perfectly

Copy link
Member

Choose a reason for hiding this comment

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

The philosophy behind this change is that the language package is designed to be used like a runtime independent library. For example, the CLI package makes use of that as well, without running a whole language server.

Then, every consumer can make use of the library as they like. Some build a VS Code extension, others a web project or a CLI. In my mind all of these packages should provide their own entry point. I think that's reasonable as a default for the generated project.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. I don't see why it can't be both a library and still expose an endpoint, but it's not a big deal, it's easy to duplicate that file. The only (minor) issue in this case is having to make the JetBrains plugin a JavaScript module as well (as in, it will have both package.json and the gradle stuff) to compile the JS file (with the exact same content as extension/src/language/main.ts) instead of just copying it from language/out/main.cjs (for example).

Copy link
Member

Choose a reason for hiding this comment

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

For those cases where a build artifact is required by multiple consumers, we usually just copy it around via some build task/script using shx or gulp. I think the generated build is fine as it is. It's not like we prevent adopters from changing how their build works after yeoman has done its job.

Copy link
Member

Choose a reason for hiding this comment

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

I see, but the only problem here could be in the (admittedly rare) occasion that someone does not generate a VS Code extension, and opts only for JetBrains plugin (for example, once that is ready).
It's not a big deal, I just need to decide whether to copy that language/main.ts to the new submodule and make it an npm package as well, or just depend on extension (VS Code) always existing.

It's not like we prevent adopters from changing how their build works after yeoman has done its job

Fair enough. Feel free to resolve this comment.

@kaisalmen
Copy link
Contributor Author

@aabounegm thank you for those additional findings/comments. Regarding vsce I selected option 2, modified the gitignore and fixed the prebublish/lint

@kaisalmen kaisalmen mentioned this pull request Aug 8, 2024
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.

Split generated yeoman project into npm workspace
8 participants