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

Use useEmberModule from modules plugin to deprecate global Ember #382

Closed

Conversation

@NullVoxPopuli NullVoxPopuli changed the title Deprecate Global Ember via new options to the modules plugin Use useEmberModule from modules plugin to deprecate global Ember Feb 22, 2021
@NullVoxPopuli
Copy link
Contributor Author

I don't know how to test this, since we are requireing packages in the ember-plugins file

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

We can test with fixturify-project

lib/ember-plugins.js Outdated Show resolved Hide resolved
@patocallaghan
Copy link
Contributor

👋 @rwjblue I spoke to @NullVoxPopuli and I offered to get this PR over the line for him. I just have a question.

I see we've already updated ember-plugins.js to use useEmberModule but do we also need to add useEmberModule to babel-options-util.js too? What's the difference between the two files/usages?

https://github.com/babel/ember-cli-babel/blob/20924faa529cf4f9c6f7cb8fa2c89d968184b620/lib/babel-options-util.js#L132

@patocallaghan patocallaghan force-pushed the deprecate-global-ember branch from 417af43 to 490f27b Compare March 6, 2021 01:31
@patocallaghan
Copy link
Contributor

@rwjblue @NullVoxPopuli I've added a test to node-tests/addon-test.js which was set up already to use fixturify-project. It seems though these tests don't exercise the logic in lib/ember-plugins.js so I had to add the useEmberModule logic to lib/babel-options-util.js too.

@patocallaghan
Copy link
Contributor

patocallaghan commented Mar 6, 2021

hmm, in the floating dependencies CI tests, i.e. yarn install --no-lockfile, the ember module gets transpiled to _ember2 rather than _ember. I'm also able to reproduce this locally.

image

Any idea how the name of the AMD module is determined? why would it appear as _ember2 in this scenario?

@rwjblue
Copy link
Member

rwjblue commented Mar 7, 2021

@patocallaghan - I think we should bump the babel-plugin-* that we have as dependencies, those failures are due to an update in one of those and bumping + fixing the fixtures in another PR should unblock.

@patocallaghan
Copy link
Contributor

I've put together a PR in #385 which upgrades some deps. It mimics what we see with the "floating deps" in CI so it will resolve our issue

@rwjblue
Copy link
Member

rwjblue commented Mar 7, 2021

Hmm, I expected that to fail. Perhaps the issue isn't with one of those plugins?

@patocallaghan
Copy link
Contributor

Ah no it is. Once I rebase this PR against that branch it starts failing. I've updated locally with _ember2 and it works fine

@patocallaghan
Copy link
Contributor

I should say nothing else fails though

@patocallaghan patocallaghan force-pushed the deprecate-global-ember branch from 490f27b to 07ea140 Compare March 10, 2021 15:45
@patocallaghan
Copy link
Contributor

@rwjblue I've rebased this against master to pick up the package upgrades. CI looks happy now 🎉

@patocallaghan
Copy link
Contributor

@rwjblue any chance this could be merged and released please? I need this and the package upgrade PR in some other repo PRs

@rwjblue
Copy link
Member

rwjblue commented Mar 17, 2021

Closing in favor of #387

@rwjblue rwjblue closed this Mar 17, 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.

4 participants