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

Move to addon v2 #66

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Move to addon v2 #66

wants to merge 9 commits into from

Conversation

ctjhoa
Copy link

@ctjhoa ctjhoa commented Oct 11, 2022

This addon is already in ember-auto-import v2 so it should not be a breaking change.
⚠️ This is breaking change
Yes this addon is already ember-auto-import v2 but ember app must be [email protected] minimum to fix the @glimmer/validator issue (cf #66 (comment))

This is based on addon v2 with yarn monorepo setup.
It follows

package.json Outdated Show resolved Hide resolved
@ctjhoa ctjhoa force-pushed the addon_v2 branch 2 times, most recently from a862812 to 831662d Compare October 13, 2022 09:57
@ctjhoa
Copy link
Author

ctjhoa commented Oct 13, 2022

I'm stuck with this PR.
There is an issue with @glimmer/validator which I'll try to sum up.

Ember embed it's own @glimmer/validator version as devDependencies.
So ember-source as a package doesn't have an explicit dependency on @glimmer/validator.

On the other side, since #54, ember-render-modifier is dependent on @glimmer/validator.

In classic (non-embroider) build, @glimmer/validator is somehow resolved as the Ember internal dependency.

Embroider, on the other hand, relies on explicit npm dependency and expects @glimmer/validator to be explicity provided by ember-source.
This result in @glimmer/validator loaded twice (in Ember itself and from the addon)

2022-10-11-144941_1916x696_scrot

Here is a list of things I've tried and do not work at the moment:

  • Declare @glimmer/validator as optional peerDependencies
  • Use yarn resolutions to force only 1 version of @glimmer/validator
  • Tweak @embroider/shared-internals ember-standard-modules.ts to add emberVirtualPackages.add('@glimmer/validator')

@ctjhoa ctjhoa requested a review from SergeAstapov October 13, 2022 09:58
@ctjhoa
Copy link
Author

ctjhoa commented Oct 13, 2022

embroider-build/ember-auto-import#541 might solve the issue

@ctjhoa
Copy link
Author

ctjhoa commented Oct 14, 2022

embroider-build/ember-auto-import#541 might solve the issue

I confirm this is working. I'm waiting for a new release of ember-auto-import

@SergeAstapov
Copy link
Contributor

@ctjhoa minor, I think we don't need packages/ember-render-modifiers/config/environment.js file at all

"release": "release-it"
},
"volta": {
"node": "16.14.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

@ctjhoa I think this should stay as 12 as minimum supported version and the same what was used in CI.

Or we could remove engines.node from @ember/render-modifiers as it's purely frontend code and has nothing to do with Node.js versions anymore (fully delegated to build pipeline)

Copy link
Author

Choose a reason for hiding this comment

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

I've removed engines.node
226ff16

As this PR introduces a breaking change, go for the latest Node LTS seems fine to me.

@ctjhoa ctjhoa marked this pull request as ready for review October 17, 2022 08:46
@ctjhoa ctjhoa requested a review from SergeAstapov October 17, 2022 08:46
@NullVoxPopuli
Copy link
Contributor

@SergeAstapov time for re-review?

@jrjohnson
Copy link
Contributor

@ctjhoa - are you still able to work on this? I think we should be able to get it over the finish line now that we've dropped support for node < 18 and Ember < 4.12.

@ctjhoa
Copy link
Author

ctjhoa commented Dec 26, 2024

@ctjhoa - are you still able to work on this? I think we should be able to get it over the finish line now that we've dropped support for node < 18 and Ember < 4.12.

Nope sorry I don't work on ember stack anymore, I've opened this PR 2 years ago and I didn't follow ember newest changes since then 🤷

@SergeAstapov
Copy link
Contributor

@SergeAstapov time for re-review?

I guess a rebase is needed first. Or a new PR forked from this branch

@jrjohnson
Copy link
Contributor

I thought I hadn't seen your name on anything for a bit @ctjhoa, thanks for the quick response. I'll take over this work and restart it either with a rebase or a fresh start. Will probably do a pnpm migration first though.

@jrjohnson
Copy link
Contributor

I'm still on the hook for this. It's on hold as I'm not having any luck finding anyone who can release this addon. Want to do a release of current master and then get some changes in.

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