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

test-setup: use caret version modifier for Embroider dependencies #1328

Merged
merged 1 commit into from
Jan 24, 2023

Conversation

simonihmig
Copy link
Collaborator

@simonihmig simonihmig commented Jan 16, 2023

When all monorepo packages were released in-sync, this ensured test-setup is referring always to the latest. But with the release process decoupled now, test-setup would effectively pull in an older pinned version of Embroider. This adds the caret modifier to allow pulling the latest version (on the same major).

Fixes #1268.

When all monorepo packages were released in-sync, this ensured test-util is refering always to the latest. But with the release process decoupled now, test-util would effectively pull in an older pinned version of Embroider. This adds the caret modifier to allow pulling the latest version (on the same major).

Fixes #1268.
@simonihmig
Copy link
Collaborator Author

Tbh, I haven't tested this, given that there are no tests and no straightforward way to test this manually (is there?). But the change seems small enough to be confident it is ok, hopefully?

@simonihmig
Copy link
Collaborator Author

Actually, with latest Embroider at 2.0, I am not so sure anymore if that is the right approach. If there is no real connection between the (major) version of test-setup and the main Embroider packages, should we rather pull in the very latest of Embroider, regardless of test-setup's own version (i.e. *)?

@NullVoxPopuli
Copy link
Collaborator

NullVoxPopuli commented Jan 16, 2023

I'd prefer defaulting to latest, with an option to specify the version of the webpack + compat + core versions (which are all co-released, afaict)

maybe for example:

 embroiderOptimized('latest', { // or version omitted to mean "latest" (today's behavior should still be supported)
  name: 'ember-lts-4.8 + embroider-optimized (latest)',
  npm: {
    devDependencies: {
      'ember-source': '~4.8.0',
    },
  },
}),
embroiderOptimized('^1.9.0', {
  name: 'ember-lts-4.8 + embroider-optimized (^1.9.0)',
  npm: {
    devDependencies: {
      'ember-source': '~4.8.0',
    },
  },
}),

This way, it's at least clear which versions will be used? and test-setup can remain a utility package and not need to worry about being released once we get test-setup to a point where it clearly supports everything, ya know?

@simonihmig
Copy link
Collaborator Author

Yeah, but you can already pass customisation options to specify the exact versions of dependencies that you want, as I did here to workaround that issue for now.

The new arguments ('latest') you proposed here would be a breaking change, not sure if that is worth it?

I think we should primarily decide on what the best default behaviour would be: latest or same major, not?
If people want to override that, then they already can...

which are all co-released, afaict

Well, but that's just how it turned out so far, that's not set in stone, right? I don't think we should rely on that.

@NullVoxPopuli
Copy link
Collaborator

The new arguments ('latest') you proposed here would be a breaking change, not sure if that is worth it?

nah, no breaking change, just an overload.

I think we should primarily decide on what the best default behaviour would be: latest or same major, not?

latest makes sense as a default to me

If people want to override that, then they already can...

yeah, but it's quite a few extra lines to do so

Well, but that's just how it turned out so far, that's not set in stone, right? I don't think we should rely on that.

yeah -- no clue if it'll stay that way, and that would be the defining limitation of what I proposed

Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

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

better than pinning, imo!

though, this could be a breaking change

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.

Missing helper: unique-id with embroider-optimized
3 participants