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

Updated modifier to use new ember-modifier api. #551

Closed

Conversation

cah-brian-gantzler
Copy link

@cah-brian-gantzler cah-brian-gantzler commented Apr 19, 2023

Expanded ember-modifier dependency to ^3.2.7 and ^4.0.0

Fixes #550

@cah-brian-gantzler
Copy link
Author

Hmmm, all this ran locally because I have node 19. Looks like would have to update the job runner from node 14 to node 16.

Ember modifier doesnt say Node 16 or greater in its docs

Ember.js v3.24 or above
Ember CLI v3.24 or above
Embroider or ember-auto-import v2.0.0 or above (this is [v2 addon](https://emberjs.github.io/rfcs/0507-embroider-v2-package-format.html))

But its volta say that

 "volta": {
    "node": "16.19.1",
    "yarn": "1.22.19"

Do we want a separate commit that updates node to 16 or greater? or do it in this PR? Seems that would warrant a major version bump as well

@cah-brian-gantzler
Copy link
Author

Any suggestions on how to handle a needed update to node 16? Do in this PR or do as a separate PR?

@cah-brian-gantzler
Copy link
Author

All the tests run locally, but I have node 16+ Attempted to update to node 16 here with this PR #552 but no ideas how to resolve the issues

Copy link
Contributor

@MelSumner MelSumner left a comment

Choose a reason for hiding this comment

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

Thank you for doing this!

@cah-brian-gantzler
Copy link
Author

Thank you for doing this!

Cant merge because the tests failed (node 16). All the tests run locally.

@cah-brian-gantzler
Copy link
Author

@MelSumner thanks for the approval, but looks like I dont have the auth to merge. And note, if it is merged, wont build til the CI is upgraded to node 16+. which I tried to do in this PR #552 but no idea what the errors are trying to say.

So merging this comes with a requirement.

@cah-brian-gantzler
Copy link
Author

cah-brian-gantzler commented Jun 16, 2023

What do I need to do to get this merged and released.

@cah-brian-gantzler
Copy link
Author

Looks like this could be a duplicate of #540 which never got merged.

What can we do about getting one of these merged? @MelSumner approved this, but not sure where to go from here

@MelSumner
Copy link
Contributor

I’m so sorry for missing this. I merged the other one, can you rebase? I’ll keep closer watch.

@cah-brian-gantzler
Copy link
Author

I am assuming that since you merged #540 this would be unneeded. Will check and close

@cah-brian-gantzler
Copy link
Author

540 does the same thing I was doing. Closing this. With it being a V2 addon, no real way of using this til you release

@corrspt
Copy link

corrspt commented Oct 13, 2023

I just go bit by this while upgrading an old app. Thanks for the tips.
I did try to put in package.json the link to the master branch (I've done that in the past with a few addons without a release). But it didn't work, I'm guessing that @cah-brian-gantzler 's comment means that in V2 addons I can't really do that anymore.

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.

Update ember-modifier to allow 4.x
3 participants