-
Notifications
You must be signed in to change notification settings - Fork 12
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
peerDep correctness #50
Conversation
BTW: along with this, I also plan to add an option to |
CI is not running here for some reason but I ran is over here: ef4#1 |
This is working now, both in the test suite here and in embroider's test suite where we first hit this problem. There are three changes in this PR:
Above I mentioned special support for undeclared peer deps, but I realized we don't need it because fixturify-project already gives us the tools to fix that. For example, here's how I patched up ember-engines and ember-asset-loader: // Both ember-engines and its dependency ember-asset-loader have undeclared
// peerDependencies on ember-cli.
function emberEngines(): Project {
let enginesPath = dirname(require.resolve('ember-engines/package.json'));
let engines = Project.fromDir(enginesPath, { linkDeps: true });
engines.pkg.peerDependencies = Object.assign(
{
'ember-cli': '*',
},
engines.pkg.peerDependencies
);
let assetLoader = Project.fromDir(dirname(require.resolve('ember-asset-loader', { paths: [enginesPath] })), {
linkDeps: true,
});
assetLoader.pkg.peerDependencies = Object.assign(
{
'ember-cli': '*',
},
assetLoader.pkg.peerDependencies
);
engines.addDependency(assetLoader);
return engines;
}
app.addDependency(emberEngines()); |
This is probably a breaking change due to |
Our CI passes on windows, but when testing this in embroider I hit a windows issue: sometimes the original and temp copies of the package are on different devices and the hardlinking throws. It would probably be OK to fall back to copying in that case. |
That may not even be a windows-only issue, I suppose it would break on unix too, possibly it just hasn't come up because our test setups on unix don't span devices. |
Confirmed I can break it on unix too. The condition is We could either catch this and error with an explanation that people should pick a different target location, or we can catch this and fall back to copying. I don't actually think copying would be ruinously expensive -- it's not the whole node modules graph you're copying, just top-level dependencies with peer deps. |
a223bb2
to
79b7114
Compare
79b7114
to
5701f90
Compare
index.ts
Outdated
if (!depTarget) { | ||
throw new Error(`package ${name} in ${target} depends on ${depName} but we could not resolve it`); | ||
} | ||
fs.ensureSymlinkSync( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although fs.ensure*
methods can be handy, i find they can easily paper over actual issues (both correctness and performance). Is it ok if we are more explicit, and explicitly create dir paths before linking into them? Reading this code, it should be totally known when it is correct to do so.
But let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really want to change this. The ensure methods make us more robust to unexpected things on the filesystem, and we don't really have sole control over the filesystem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thats ok if you feel strongly.
In this case, it is a region of the FS that we do have control over. But its not a big deal.
A peer dep means you get the same copy as your parent. It doesn’t imply
going any higher. The example you sketched probably doesn’t really work
reliably under all workspace scenarios. For c to see a’s copy of e, b needs
to also declare the peerDep.
…On Mon, Sep 13, 2021 at 2:33 PM Stefan Penner ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In index.ts
<#50 (comment)>
:
> @@ -258,6 +256,56 @@ export class Project {
dep.baseDir = path.join(this.baseDir, 'node_modules', dep.name);
dep.writeSync();
}
+ for (let [name, { dir: target }] of this.dependencyLinks) {
+ this.writeLinkedPackage(name, target);
+ }
+ }
+
+ private writeLinkedPackage(name: string, target: string) {
+ let targetPkg = require(path.join(target, 'package.json'));
+ let peers = new Set(Object.keys(targetPkg.peerDependencies ?? {}));
+ let destination = path.join(this.baseDir, 'node_modules', name);
+
+ if (peers.size === 0) {
What happens when a given linked dependency itself has no
peer-dependencies, but one of it's dependencies does?
Scenario:
a -> e
a -> b -> c -[peer]-> e
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#50 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACN6MQSZIARS46QJFULVOTUBY7XRANCNFSM5DZSOOTA>
.
|
@ef4 when thinking about this problem, my initial idea was to use |
@ef4 I can accept that, but this makes me realize that I may be operating on a different definition of how peerDeps in our ecosystem are meant to work. To correct this at some point, we should sync up and sketch-out a unified understand of what & how peer deps work in our world. That way, I'm operating on a similar definition. |
Yeah, NPM has been unhelpfully vague about peerDep formal semantics. Yarn has documented things more carefully in the course of writing their PnP RFC. For example, one of the formal guarantees is:
|
This is now tested and fully working in Embroider in embroider-build/embroider#961. I'm testing it in ember-auto-import as well and have uncovered failures. Possibly they are real bugs that were hidden by this bug. https://github.com/ef4/ember-auto-import/pull/447/checks?check_run_id=3591884331 |
This is now also working in ember-auto-import. |
We need to ensure that linked packages see the correct peerDeps.
(EDITED: previous description here suggested an incorrect solution.)