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

Add dependenciesMeta.*.injected, fix peer deps #49

Merged
merged 2 commits into from Feb 15, 2023
Merged

Conversation

simonihmig
Copy link
Contributor

Latest PRs like #45 and #44 are failing CI. After debugging, it seems again related to embroider-build/embroider#1332, in that dependencySatisfies (in ember-modifier this time) is seeing a wrong package of ember-source (that is unused in test-app, thus going into that empty package case).

While a previous attempt to fix this by using dependenciesMeta.*.injected before didn't succeed, it seems to do so now!

Also fixing peer deps now, as with injected the workaround of not having ember-source as a devDependency seems to not be needed anymore.

@changeset-bot
Copy link

changeset-bot bot commented Feb 15, 2023

⚠️ No Changeset found

Latest commit: e988c6d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@simonihmig
Copy link
Contributor Author

I believe CI is failing as we are building the /dist output after pnpm install, but then the now hard-linked packages are not synced, so /dist is missing. Am I right about this @NullVoxPopuli?

If so, I don't think it's worth it trying to fix this here just to make CI pass, as #45 will refactor the whole setup. But then both PRs are required to make CI green, which means we need to merge one (this?) in a red state and bring the changes into the other to (hopefully!) make things green again. Ok?

@@ -31,7 +31,8 @@
"peerDependencies": {
"validated-changeset": "^1.3.4",
"ember-changeset": "^4.1.2",
"ember-headless-form": "^0.0.0"
"ember-headless-form": "^0.0.0",
"ember-source": "^4.8.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the min version 4.4 or 4.8?

The try config has 4.4. last i looked, but if 4.8 is min required, then we can remove the function polyfill 🎉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, good catch. Yeah, I still have 4.4, that's why the polyfill was added. Accidental copy-paste error, will fix!

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Feb 15, 2023

Am I right about this @NullVoxPopuli?

Yup, the download-built-package action, if it's still used, needs to run pnpm i -f again

Ok

Yup!

#45

This approach, and turbo in general won't work until injected dependencies can work without pnpm i -f

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

3 participants