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

Switch to pnpm and restore @ember/test-helpers to latest patch #894

Merged
merged 12 commits into from
Jan 21, 2023

Conversation

gilest
Copy link
Collaborator

@gilest gilest commented Jan 14, 2023

Allows us to continue to test Ember 3.28 via ember try.

I've been struggling with that particular build since: emberjs/ember-test-helpers#1232

To my best understanding a quirk in yarn's (incorrect) handling of peerDependencies with monorepo causes incorrect resolution of ember-source.

I wasn't able to find a nohoist config with yarn which worked, although I'm open to suggestions.

As suggested in this comment pnpm handles peerDeps more correctly, and provides the dependenciesMeta.*.injected option.

This issue has been blocking the release of ember-file-upload because this addon includes @ember/test-helpers in its own dependencies (we ship custom test helpers which depend on that package). I don't want to cut a release with a restrictive @ember/test-helpers dependency version.

@gilest gilest force-pushed the chore/switch-to-pnpm branch from 76ff0b5 to 7acd723 Compare January 14, 2023 23:47
@gilest gilest force-pushed the chore/switch-to-pnpm branch from 7acd723 to 1a316ae Compare January 14, 2023 23:48
Copy link
Collaborator

@jelhan jelhan left a comment

Choose a reason for hiding this comment

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

Looks good to me. But I'm not a pnpm expert.

@@ -63,12 +63,16 @@
"@babel/plugin-transform-runtime": "^7.18.2",
"@babel/plugin-transform-typescript": "^7.18.4",
"@babel/preset-typescript": "^7.17.12",
"@babel/runtime": "^7.20.7",
"@ember/string": "^3.0.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This may need to go into peerDependency to avoid a deprecation in Ember 4.10+. Adding as peer dependency would be a breaking change. Maybe better done in separate PR.

When doing a new major for that, we should also revisit other dependencies and see if they should be peer dependencies instead. E.g. @glimmer/tracking and @glimmer/component.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah good point. Definitely better done in a separate PR!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a tracking issue #899

@gilest gilest changed the title Switch to pnpm Switch to pnpm and restore @ember/test-helpers to latest patch Jan 21, 2023
@gilest gilest mentioned this pull request Jan 21, 2023
@gilest gilest merged commit 735cc25 into master Jan 21, 2023
@gilest gilest deleted the chore/switch-to-pnpm branch October 23, 2023 08:16
@gilest gilest mentioned this pull request Dec 8, 2023
gilest added a commit that referenced this pull request Dec 8, 2023
Some config adjustments since #1031

The private packages version numbers and dependency on ember-file-upload are being bumped. This causes the CI to fail after each release because a new lockfile was not generated

We also want to keep the workspace:* dependency since we're using dependenciesMeta.

We're using dependenciesMeta to get the best possible peerDependency resolution:

Switch to pnpm and restore @ember/test-helpers to latest patch #894
Fix docs website peer dependency resolution #968
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants