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

Proposed ember-moment v10 #371

Merged
merged 25 commits into from
Jan 15, 2022
Merged

Proposed ember-moment v10 #371

merged 25 commits into from
Jan 15, 2022

Conversation

ef4
Copy link
Collaborator

@ef4 ef4 commented Jan 8, 2022

The switch from ember-cli-moment-shim to ember-auto-import was a good step (ember-auto-import is more future-aligned with general ES-module-compatible build systems). But the way ember-moment currently (9.x) brings both moment and moment-timezone as dependencies and auto-imports them is problematic.

  1. For the app and ember-moment to both gain access to moment and guarantee that it's the exact same copy, moment should be a peerDep of ember-moment, not a dep.
  2. It's not correct to depend on both moment and moment-timezone. moment-timezone already has moment as a dependency. It's one or the other.
  3. When consuming moment or moment-timezone via webpack (which is what ember-auto-import is doing), there are extra steps needed to control the locales and timezone data:

There are several other things I will be addressing here too:

  • Ember 4.0 compatibility
  • Providing utilities that app authors should use to take care of the aforementioned webpack settings under either ember-auto-import or embroider
  • Detecting the presence of ember-cli-moment-shim and warning app authors that it's giving them a duplicate copy of moment (I think a lot of real apps are currently shipping moment twice without realizing it!).

This PR is a breaking change that would be ember-moment 10.0. The breaking changes are:

  • apps must add moment or moment-timezone (to satisfy the new peerDep requirement)
  • apps must have ember-auto-import >= 2 (since we're doing a breaking release anyway, this is a good time to convert ember-moment to v2 format, which means consuming apps need ember-auto-import >=2).

Big picture: moment is an end-of-life technology. My recommendation to app authors is not to rush into rewriting from moment to another library like Luxon, because Temporal is very close to happening, and if you're going to port you might as well port directly to Temporal (which will have native browser implementations).

@ef4
Copy link
Collaborator Author

ef4 commented Jan 8, 2022

(This was intended to be a WIP PR, not done yet.)

@ef4 ef4 changed the title V2 addon Proposed ember-moment v10 Jan 9, 2022
@ef4
Copy link
Collaborator Author

ef4 commented Jan 9, 2022

@alexlafroscia @NullVoxPopuli I'm willing to be added a maintainer here to land this work and then support it.

package.json Outdated
"ember-cli-3.16": "npm:ember-cli@~3.16.0",
"ember-cli-3.20": "npm:ember-cli@~3.20.0",
"ember-cli-3.24": "npm:ember-cli@~3.24.0",
"ember-cli-3.28": "npm:ember-cli@~3.24.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"ember-cli-3.28": "npm:ember-cli@~3.24.0",
"ember-cli-3.28": "npm:ember-cli@~3.28.0",

});
}

// async function release(project) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

how come these are commented out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's still WIP. I intend to get those passing before this is ready.

@NullVoxPopuli
Copy link
Collaborator

This is exciting work! thanks for this!

@ef4 ef4 merged commit c80dd17 into adopted-ember-addons:master Jan 15, 2022
@ef4
Copy link
Collaborator Author

ef4 commented Jan 15, 2022

This is ready to release. Can I get auth on npm please?

@NullVoxPopuli
Copy link
Collaborator

Done

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.

None yet

2 participants