adopted-ember-addons / ember-pikaday

A datepicker component for Ember CLI projects.
MIT License
159 stars 169 forks source link

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

Closed cah-brian-gantzler closed 1 year ago

cah-brian-gantzler commented 1 year ago

Expanded ember-modifier dependency to ^3.2.7 and ^4.0.0

Fixes https://github.com/adopted-ember-addons/ember-pikaday/issues/550

cah-brian-gantzler commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

All the tests run locally, but I have node 16+ Attempted to update to node 16 here with this PR https://github.com/adopted-ember-addons/ember-pikaday/pull/552 but no ideas how to resolve the issues

cah-brian-gantzler commented 1 year ago

Thank you for doing this!

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

cah-brian-gantzler commented 1 year ago

@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 https://github.com/adopted-ember-addons/ember-pikaday/pull/552 but no idea what the errors are trying to say.

So merging this comes with a requirement.

cah-brian-gantzler commented 1 year ago

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

cah-brian-gantzler commented 1 year ago

Looks like this could be a duplicate of https://github.com/adopted-ember-addons/ember-pikaday/pull/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 commented 1 year ago

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

cah-brian-gantzler commented 1 year ago

I am assuming that since you merged https://github.com/adopted-ember-addons/ember-pikaday/pull/540 this would be unneeded. Will check and close

cah-brian-gantzler commented 1 year ago

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 commented 10 months ago

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.