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

Tree-shaking via only / except configuration is broken in v4 #1323

Open
jelhan opened this issue Jan 2, 2024 · 5 comments
Open

Tree-shaking via only / except configuration is broken in v4 #1323

jelhan opened this issue Jan 2, 2024 · 5 comments

Comments

@jelhan
Copy link
Contributor

jelhan commented Jan 2, 2024

The tree-shaking via only and except configuration options does not seem to work anymore in 4.0.0. I think it was dropped when converting the addon to v2 in #945.

I assume this was done by accident as the feature is still documented:

## Configuration
If you don't need all the helpers, you can specify which to whitelist or blacklist using `only` or `except` within your `ember-cli-build.js`:
```js
module.exports = function(defaults) {
var app = new EmberApp(defaults, {
'ember-math-helpers': {
only: ['add', 'sub'],
except: ['random', 'tan']
}
});
```
Both `only` and `except` can be safely used together (the addon computes the diff), although it's best if you only use one for your own sanity.
```js
except: ['random'] // imports all helpers except `random`
only: ['random'] // imports only `random`
```

This breaking change is also not listed in the changelog:

## v4.0.0 (2023-11-02)
#### :boom: Breaking Change
* [#965](https://github.com/RobbieTheWagner/ember-math-helpers/pull/965) Support Ember 5, drop Ember 3.x support ([@RobbieTheWagner](https://github.com/RobbieTheWagner))
#### :rocket: Enhancement
* [#1163](https://github.com/RobbieTheWagner/ember-math-helpers/pull/1163) Add full TS/glint support ([@RobbieTheWagner](https://github.com/RobbieTheWagner))
* [#1162](https://github.com/RobbieTheWagner/ember-math-helpers/pull/1162) Type declarations and template registry for Glint ([@BoussonKarel](https://github.com/BoussonKarel))
#### :memo: Documentation
* [#951](https://github.com/RobbieTheWagner/ember-math-helpers/pull/951) Fix broken URLs ([@ursm](https://github.com/ursm))
#### :house: Internal
* [#1100](https://github.com/RobbieTheWagner/ember-math-helpers/pull/1100) Fix build ([@RobbieTheWagner](https://github.com/RobbieTheWagner))
#### Committers: 3
- Keita Urashima ([@ursm](https://github.com/ursm))
- Robbie Wagner ([@RobbieTheWagner](https://github.com/RobbieTheWagner))
- [@BoussonKarel](https://github.com/BoussonKarel)

I noticed this when investigating if we can convert Ember Bootstrap to a v2 addon without dropping support for tree-shaking through configuration.

@RobbieTheWagner
Copy link
Owner

@jelhan you are correct that support was dropped for it. I believe tree shaking should be automatic with embroider and only use the things you import, but I could be wrong.

@jelhan
Copy link
Contributor Author

jelhan commented Jan 5, 2024

I believe tree shaking should be automatic with embroider and only use the things you import, but I could be wrong.

That's only true if consuming app uses Embroider and opted in to { staticHelpers: true } as far as I know.

@RobbieTheWagner
Copy link
Owner

I believe tree shaking should be automatic with embroider and only use the things you import, but I could be wrong.

That's only true if consuming app uses Embroider and opted in to { staticHelpers: true } as far as I know.

That's possible, but that is the future, so I don't know if we need configs for this anymore in this addon.

@jelhan
Copy link
Contributor Author

jelhan commented Jan 5, 2024

I don't know if we need configs for this anymore in this addon.

I think it's a maintainer decision. I see two options:

  1. Document the breaking change in the changelog and update the docs. Or
  2. Rollback the v2 addon conversion and restore the functionality as a bug fix.

A third one would be restoring the functionality for apps not using Embroider. This is discussed here: embroider-build/embroider#1748 (comment) It would be still a breaking change introduced in v4 but affecting a smaller subset of apps using this addon.

@RobbieTheWagner
Copy link
Owner

I think documenting the breaking change is probably the best way to go here.

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

No branches or pull requests

2 participants