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

[BUGFIX beta] Add ember-auto-import 2 #19761

Merged
merged 1 commit into from Sep 27, 2021
Merged

[BUGFIX beta] Add ember-auto-import 2 #19761

merged 1 commit into from Sep 27, 2021

Conversation

ef4
Copy link
Contributor

@ef4 ef4 commented Sep 24, 2021

Closes #19752

This solves that because ember-auto-import 2 already provides the required assertion.

@rwjblue rwjblue changed the title Add ember-auto-import 2 [BUGFIX beta] Add ember-auto-import 2 Sep 27, 2021
@rwjblue rwjblue merged commit 3549ad9 into master Sep 27, 2021
@rwjblue rwjblue deleted the eai2 branch September 27, 2021 16:25
@simonihmig
Copy link
Contributor

This causes failing Canary CI builds for addons that still use e-a-i v1: Error: To use these addons, your app needs ember-auto-import >= 2: ember-source.

I think this can be fixed by adding v2 (and webpack) to the devDependencies of the canary ember-try scenario. However that approach is a bit fragile, as the other "floating" scenarios (beta, release) would at some point also be affected by this, so the best would probably be to do that prematurely for them as well!?

This will likely cause some churn in the ecosystem, for all addons that haven't jumped to v2 yet, and maybe aren't ready yet to do that major bump. Thoughts?

@rwjblue
Copy link
Member

rwjblue commented Oct 1, 2021

I think this can be fixed by adding v2 (and webpack) to the devDependencies of the canary ember-try scenario.

I think the best fix is to add webpack@5 and ember-auto-import@2 to the devDependencies of the addon's in question. If you do this, you won't need to do anything with the ember-try scenarios at all.

That change was landed in ember-cli here ember-cli/ember-cli@20b71db, and will be released in the upcoming ember-cli@4.0.0-beta.1.

@simonihmig
Copy link
Contributor

I think the best fix is to add webpack@5 and ember-auto-import@2 to the devDependencies of the addon's in question

Will that work when the addon already has eai@1 in its dependencies? (and is not ready yet to do the major bump, but still wants a green CI)

@ef4
Copy link
Contributor Author

ef4 commented Oct 4, 2021

Will that work when the addon already has eai@1 in its dependencies?

Hmm, that's a good point. This is a place where the mixed nature of dummy apps is going to cause unnecessary pain.

What you really want is your addon having a dependency on eai 1 and your dummy app having a dependency on eai 2. That would work cleanly and let you test your addon under ember 4, without upgrading the addon's own dependency. But dummy apps mix up all the dependencies between the app and the addon. @rwjblue would know better, but I don't think there's a way to prioritize between dependencies and devDependencies such that it would have the right effect, if you use a regular dummy app.

On the other hand, the cost vs benefit to upgrading to eai 2 in your addon makes the upgrade pretty worthwhile. We've seen even very large apps upgrade with very little effort, so it's not imposing much cost on your users. Plus there is a significant build-time performance benefit starting at eai 2.2, and it scales with the number of addons that upgrade.

@simonihmig
Copy link
Contributor

This is a place where the mixed nature of dummy apps is going to cause unnecessary pain.

💯 (yet again, we have to say... 😔. )

On the other hand, the cost vs benefit to upgrading to eai 2 in your addon makes the upgrade pretty worthwhile

Yeah, I do want to do that, for the addon I was working I planned a major bump anyway. But there might be reasons for other addons to not do this yet, and those will run into those failing CI builds now. Just wanted to mention this, as it might cause some confusion for probably a lot of maintainers!?

@jherdman
Copy link
Contributor

jherdman commented Oct 4, 2021

I want to chime in as one of these maintainers. We have a bunch of internal addons with EAIv2 upgrade ticketed for "the near future" ™️ . Failing canary builds aren't a huge deal insofar that we know and understand why it's happening, but it's not fun having a bunch of failing canary builds.

sandydoo added a commit to sandydoo/ember-google-maps that referenced this pull request Oct 13, 2021
emberjs/ember.js#19761 (comment)

Apparently, we can add auto-import `>= 2` to our devDependencies
without *crosses fingers* any issue...as long as we don’t need it
in our addon code.

This is a partial revert for PR #153.
@ef4
Copy link
Contributor Author

ef4 commented Oct 13, 2021

To just get passing canary builds, the earlier suggestion to put ember-auto-import 2 and webpack inside the ember-canary ember-try scenario is a good solution:

{
  name: 'ember-canary',
  npm: {
    devDependencies: {
      'ember-source': await getChannelURL('canary'),
      'ember-auto-import': '^2.0.0',
      'webpack': '^5.0.0'
    },
  },
},

LevelbossMike added a commit to LevelbossMike/ember-statecharts that referenced this pull request Nov 20, 2021
LevelbossMike added a commit to LevelbossMike/ember-statecharts that referenced this pull request Nov 21, 2021
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.

Require ember-auto-import >= 2 or higher with ember-source@4.0.0
4 participants