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

Remove unneeded babel plugins so that shipped addon code is way smaller and easier to debug without sourcemaps #231

Merged
merged 9 commits into from
Dec 8, 2023

Conversation

NullVoxPopuli
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli commented Nov 28, 2023

(Sourcemaps are, ofc, still very good, and hi-fi)

Changes:

  • Removes: the babel plugins related to shipped proposals as well as the decorator transform
  • Adds: decorators-transform, which is a more targeted way to do legacy decorators. In apps, on average, this saves about 15% in size.

Details: https://github.com/ef4/decorator-transforms

@SergeAstapov
Copy link
Contributor

SergeAstapov commented Nov 28, 2023

Adds: decorators-transform, which is a more targeted way to do legacy decorators. In apps, on average, this saves about 15% in size.

according to embroider-build/ember-auto-import#601, this would make no difference?

Or, would it be correct to say, that it makes no difference for "classic build" via Ember CLI and would just work for apps built with Embroider?

@NullVoxPopuli
Copy link
Collaborator Author

NullVoxPopuli commented Nov 28, 2023

It's a separate problem. That issue and classic vs embroider aren't a concern for the compilation of a library -- it is true that the app's / consumer's babel will run over this code again, but addon maintainers will want less to maintain, and fewer babel plugins is always good :D

@SergeAstapov
Copy link
Contributor

@NullVoxPopuli thanks for confirming the assumption!

@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review December 8, 2023 13:03
@NullVoxPopuli
Copy link
Collaborator Author

TS tests are failing atm -- passing on main: https://github.com/embroider-build/addon-blueprint/actions/runs/7143108601 (as of 3s ago)

@@ -89,17 +89,12 @@ for (let packageManager of SUPPORTED_PACKAGE_MANAGERS) {
'template-registry.js.map',
]);

expect(distContents.hashed.length).toBe(4);
expect(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we no longer emit rollup helpers

expect(
distContents.hashed
.filter((file) => file.includes('template-only'))
.map((file) => file.split('.js')[1]),
).to.deep.equal(['', '.map'], 'the template-only component is emitted with a source map');
).to.deep.equal(['', '.map'], 'the template-only (private) component is emitted with a source map');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just clarifying here that this assertion is ensuring that private components are still present in the output

@@ -33,14 +33,12 @@
"prepack": "rollup --config"
},
"dependencies": {
"@embroider/addon-shim": "^1.0.0"
"@embroider/addon-shim": "^1.0.0",
"decorator-transforms": "^1.0.1"
Copy link
Contributor

@SergeAstapov SergeAstapov Dec 8, 2023

Choose a reason for hiding this comment

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

@NullVoxPopuli out of curiosity why would it go to dependencies? Is it intentional or mistake?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

because it has a tiny runtime module that needs to be imported

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@NullVoxPopuli NullVoxPopuli added the enhancement New feature or request label Dec 8, 2023
@ef4 ef4 merged commit c161b56 into main Dec 8, 2023
21 checks passed
@ef4 ef4 deleted the try-decorator-transforms branch December 8, 2023 18:08
@github-actions github-actions bot mentioned this pull request Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants