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

Upgrade to ember-auto-import v2 #1624

Merged
merged 1 commit into from Oct 19, 2021
Merged

Upgrade to ember-auto-import v2 #1624

merged 1 commit into from Oct 19, 2021

Conversation

simonihmig
Copy link
Contributor

This requires the app to also have eai v2. But should fix failing Canary builds (which now also requires eai v2, so requires our addon's dummy app to have eai v2 and webpack, see emberjs/ember.js#19761). And enables us to eventually ship a native v2 addon without further breaking changes.

@simonihmig simonihmig added breaking dependencies Pull requests that update a dependency file labels Oct 4, 2021
@simonihmig simonihmig requested a review from jelhan October 4, 2021 17:57
@jelhan
Copy link
Contributor

jelhan commented Oct 4, 2021

I'm wondering if it is too early to do that change. Ember CLI blueprints for new applications are still using ember-auto-import@^1.11.3 for new applications: https://github.com/ember-cli/ember-new-output/blob/ce21320b131b4c54dee9b031353a8877194b6bd5/package.json#L32 It has been updated on master branch: https://github.com/ember-cli/ember-cli/blob/ebea201b9dfe451aa6ee8678c88e883a88e03628/blueprints/app/files/package.json#L35 I guess the plan is to ship it with Ember CLI v4. But shouldn't we at least wait until that one is official released? There is still the risk that it won't be shipped even with v4, isn't it?

@simonihmig
Copy link
Contributor Author

I'm wondering if it is too early to do that change.

Too early for what? What's your specific concern?

Ember v4 (which is also an addon) already requires it. And the only breaking change is that any app depending on an addon that requires eai v2 also has to update, but that's all. What's the risk?

@jelhan
Copy link
Contributor

jelhan commented Oct 4, 2021

Ember v4 (which is also an addon) already requires it.

Does it require it? I assumed/hoped that an Ember v4 application can use an addon, which depends on ember-auto-import@v1. Otherwise the upgrade pain would be very hard. But maybe I was too optimistic. 🤞

@simonihmig
Copy link
Contributor Author

simonihmig commented Oct 5, 2021

The logic is as follows, see also https://github.com/ef4/ember-auto-import/blob/main/docs/upgrade-guide-2.0.md:

  • if any addon requires eai2, then the app also has to have it
  • an eai2 app can still depend on as many eai1 addons as you want

So it's not that all addons have to upgrade. But apps will likely "feel the pressure" to do so, once some addons do upgrade. One such "addon" is Ember 4. But upgrading should be easy and cheap, and unlocks future improvements. So I don't see a good reason to hold this back!?

And upgrading does require a major version bump, so for e-b I think now is the time to do so!

@jelhan
Copy link
Contributor

jelhan commented Oct 5, 2021

The logic is as follows, see also https://github.com/ef4/ember-auto-import/blob/main/docs/upgrade-guide-2.0.md:

* if _any_ addon requires eai2, then the app also has to have it

* an eai2 app can still depend on as many eai1 addons as you want

So it's not that all addons have to upgrade. But apps will likely "feel the pressure" to do so, once some addons do upgrade. One such "addon" is Ember 4. But upgrading should be easy and cheap, and unlocks future improvements. So I don't see a good reason to hold this back!?

I see. Thanks a lot for the explanation.

I still fear the pain of being one of the first to do that change. Do we have any benefits from doing that change now beside avoiding another major only for that reason later?

@simonihmig
Copy link
Contributor Author

I still fear the pain of being one of the first to do that change. Do we have any benefits from doing that change now beside avoiding another major only for that reason later?

I believe it could be beneficial for performance reasons but not sure.

Fun fact: I just found out that we actually don't need it as a dependency! 😝. Seems it was added in this commit, but macro-decorators was later removed. So we could also remove it. On the other hand, having v2 as a dependency does allow us to publish e-b as a v2 addon, hopefully without another breaking change, which could be a big improvement on build times etc.

Copy link
Contributor

@jelhan jelhan left a comment

Choose a reason for hiding this comment

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

Seems as other addons start upgrading to ember-auto-import@v2 - including addons we depend on. I just came across @ember/render-modifiers, which upgraded to ember-auto-import@v2 in their recent 2.0.0 release: https://github.com/emberjs/ember-render-modifiers/blob/master/CHANGELOG.md#v200-2021-10-06 Not sure if they were aware of the impact. The change is listed as internal.

I fear if not doing that breaking change now ourselves, we may not be able to upgrade some of our dependencies. We would lock ourselves into a corner.

I guess that was the part I missed so far. Seeing that risk, I'm in favor of shipping that one as part of our upcoming major.

@SergeAstapov
Copy link

@jelhan just to clarify why ember-auto-import@v2 upgrade in @ember/render-modifiers was marked as "internal":

@ember/render-modifiers does not need ember-auto-import at runtime, it's only needed to run the tests hence its listed in devDependencies and has no impact to end users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants