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

Export grammar-related generators from CLI #1434

Merged
merged 6 commits into from
Apr 22, 2024
Merged

Export grammar-related generators from CLI #1434

merged 6 commits into from
Apr 22, 2024

Conversation

Lotes
Copy link
Contributor

@Lotes Lotes commented Apr 2, 2024

For the Langium playground I need to import the Textmate generator.
Therefore I am exporting a couple of generators from the CLI.
My current alternative is to copy the code from this repo, which is actually not that nice.

Speak up if you want a different export strategy. Currently I am exporting each generator in its own export alias.


This PR adds a new ESM export /textmate to the langium-cli package. It contains the highlighting generator for Textmate syntax highlighters (Monaco editor).

@dhuebner
Copy link
Contributor

dhuebner commented Apr 2, 2024

@Lotes
In general I think it is good to export useful components to users. One thing to keep in mind, when exporting code we make it a public API, hence need to care that we do not break it in the future and we should document it.
If you just need the textmate generator, maybe we should first only export just this generator, WDYT?

@Lotes
Copy link
Contributor Author

Lotes commented Apr 2, 2024

@dhuebner Thanks for the argument. That is a very valid point. Then I will only export the Textmate generator. But even there I am unsure: Shall I export it under "/textmate" or better under something more general like "/syntax-highlighting"...? Maybe start with something specific and deprecate when the time has come and it becomes more general?

@montymxb
Copy link
Contributor

montymxb commented Apr 3, 2024

I agree with @dhuebner here. Exporting just what we need makes sense for now. And as for the name, textmate seems fine, but we should document that new endpoint to make it clear what it provide (later on, before the next release).

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Since the textmate-generator.js file imports from ../util.js, it also attempts to import node libraries such as node:path and node:url. We should split the node related utilities from the Langium related utilities, as the node imports make the ./textmate import difficult to bundle in a browser environment. It would require a lot of shims/polyfills in order to successfully bundle/run.

@Lotes
Copy link
Contributor Author

Lotes commented Apr 3, 2024

@montymxb As far as I can see there is no next change log file or something similar. So I will document the endpoint in the main description of this PR.
@msujew I have done the split. Nice catch. Please have another look.

@Lotes Lotes requested a review from msujew April 3, 2024 12:39
Copy link
Contributor

@montymxb montymxb left a comment

Choose a reason for hiding this comment

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

Looks okay from my end. Documentation is just a note, we'll handle that later of course 🙂 .

@Lotes
Copy link
Contributor Author

Lotes commented Apr 11, 2024

I need to wait for #1437 since I want to increment the Langium CLI version and trigger Langium generator for the examples.

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Just two minor suggestions, see below.

packages/langium-cli/CHANGELOG.md Outdated Show resolved Hide resolved
@Lotes Lotes requested a review from msujew April 19, 2024 19:21
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Great, looks good to me 👍

@msujew msujew merged commit 39e55c4 into main Apr 22, 2024
5 checks passed
@msujew msujew deleted the lotes/cli-exports branch April 22, 2024 13:28
@spoenemann spoenemann added this to the v3.1.0 milestone May 6, 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.

5 participants