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

Split grammar and main export #1171

Merged
merged 5 commits into from
Dec 1, 2023
Merged

Split grammar and main export #1171

merged 5 commits into from
Dec 1, 2023

Conversation

msujew
Copy link
Member

@msujew msujew commented Aug 24, 2023

Closes #1167

Bundle size benchmark on the arithmetics example:

main PR main, minified PR, minified
1308kb 1019kb 633kb 457kb

We can see that we remove roughly ~175kb from a minified language server bundle with this change, reducing small language server bundles by 28%.

Copy link
Contributor

@spoenemann spoenemann left a comment

Choose a reason for hiding this comment

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

Thanks! A few questions, maybe we can discuss them in today's dev meeting:

  • Why did you move so many functions from internal-grammar-util to grammar-util?
  • How can we guarantee that we don't accidentally introduce unwanted dependencies to grammar code in the core framework?
    • Many files use grammar/generated/ast – maybe we should move that out of the grammar folder or reexport them
    • ast-reflection-interpreter uses more grammar internal code, but is in the language folder
    • value-converter still uses a function from internal-grammar-util

@msujew msujew force-pushed the msujew/grammar-split branch 2 times, most recently from 0cc0ee1 to fcbc1a6 Compare September 22, 2023 09:28
@msujew
Copy link
Member Author

msujew commented Sep 22, 2023

Why did you move so many functions from internal-grammar-util to grammar-util?

The internal-grammar-util import is only supposed to be used from inside the langium/grammar import path. The reason for this is adopter customizability. If we import from internal-grammar-util in our default implementations (for example for LSP services), then adopters need to import from langium/grammar in case they want to override certain behavior - even though the service isn't grammar related.

Many files use grammar/generated/ast – maybe we should move that out of the grammar folder or reexport them

I would tackle this in another PR. The PR is large enough as it is.

@cdietrich
Copy link
Contributor

cdietrich commented Sep 22, 2023

@msujew we are currently using getActionAtElement too. (besides getTypeName which was moved)
can you consider moving it?

@msujew msujew added this to the v2.1.0 milestone Sep 26, 2023
@msujew msujew requested a review from spoenemann September 26, 2023 13:35
@msujew msujew force-pushed the msujew/grammar-split branch from b2a7d27 to 9d6a154 Compare September 28, 2023 12:38
Copy link
Contributor

@spoenemann spoenemann left a comment

Choose a reason for hiding this comment

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

I know we've talked about including this change in the 2.x version stream, but now it looks like a too big breaking change to me. One option could be to plan for a 3.0.0 version rather soon so we don't have to worry too much about the changes?

packages/langium/src/utils/grammar-util.ts Show resolved Hide resolved
Copy link
Contributor

@spoenemann spoenemann left a comment

Choose a reason for hiding this comment

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

The changes look good to me. I would only suggest to modify the main package export test so it excludes the grammar code (see #1269).

I also thought about generating the grammar related files into different folders, specifically to move the AST file to src/language/grammar-ast.ts. This would make code isolation much easier, but we would need a special case for langium-cli (which we already have with the langiumInternal flag). WDYT?

Copy link
Contributor

@sailingKieler sailingKieler left a comment

Choose a reason for hiding this comment

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

Looks very good to me 👍, I have only one remark on the duality of grammar vs. language about making clear its rationale. IMO the name language is too ambiguous here, what about languages or language-api. Or even grammar-api as it describes the API to implement evaluations of grammars.

I assume you have tested that this cut get's us the expected reduction in bundle size 😉

packages/langium/src/language/index.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@sailingKieler sailingKieler left a comment

Choose a reason for hiding this comment

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

As I already said, some hints in the index files of languages and grammar on their purpose would be nice for others to understand their purpose.

Apart from that 🚀

@msujew msujew merged commit 729eeed into main Dec 1, 2023
5 checks passed
@msujew msujew deleted the msujew/grammar-split branch December 1, 2023 14:40
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.

Export grammar language via dedicated ESM export
5 participants