Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

[Proposed] Auto-detect if Package is "User-Owned" or not #1876

Merged
merged 2 commits into from
Mar 26, 2020

Conversation

warrenfalk
Copy link
Contributor

OK, Here is my proposed solution for supporting "user-owned" packages other than the "default" package.

Note: sometimes these are called "local" packages or just "not external" packages, but in the code, "local" and "external" can sometimes mean other things. So I'm using the term "user-owned" to mean "A package that we expect the user to modify". This is in contrast to packages that are downloaded from a package registry (things in node_modules) that the user should not modify. I derived the term from PackageType.USER_PACKAGE which is already in the code.

Fuse-box already has a package type that can be either USER_PACKAGE or EXTERNAL_PACKAGE. The type determines how the package is treated by the cache. But currently only the "default" package is ever set to USER_PACKAGE. And so only the one "default" package is ever considered to be user-owned.

This has been a problem for monorepo setups in particular where the user-owned code is itself split up into packages. The user will routinely change files outside of the "default" package and wants these things to break cache and to trigger HMR just like changes inside the "default" package do. But currently they don't because only package.json changes break cache if type is not USER_PACKAGE.

My proposal is basically this: If a package's true location is outside of node_modules then treat it as user-owned. "True location" means: where it really is (after symlinks are dereferenced).

(Yarn 2 Note: There is no node_modules in Yarn 2, so instead we resolving any PnP virtual path and seeing if that is a real directory, and if it is, then we consider that user-owned).

I think this is likely to resolve the problem @starsolaris is trying to solve with PR #1844 for, but does so automatically without requiring manual configuration. Note: that we could also later support a manual override, but if we did I think it should just override detection of "user/non-user". But I suspect that this solves @starsolaris's problem without any need to modify config.

This is currently split up into two commits. It is easiest to review them separately. The first one just refactors the way fileLookup() and nodeModuleLookup() return errors. The feature I am proposing is in the second commit.

@warrenfalk
Copy link
Contributor Author

I'm looking into why the CI test fails. The local test is working, need to see if this is a bug in the test or a real bug.

@warrenfalk warrenfalk force-pushed the feature/user-module-detection branch from 5f5dec0 to 7e167f9 Compare March 25, 2020 14:11
@warrenfalk
Copy link
Contributor Author

OK, was a bug in the test; that's fixed. Local worked because of some files created by previous tests that are no longer created but hadn't been cleaned up.

@nchanged
Copy link
Contributor

nchanged commented Mar 25, 2020

@wagerfield one test doesn't pass on my local machine. I sent you a message on Slack earlier

Copy link
Contributor

@nchanged nchanged left a comment

Choose a reason for hiding this comment

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

That one actually looks very good. I don't have anything to add. There is only one question I have. If yarn workspaces uses symlinks, how does it become "user-owned" in general? Is it because we can now detect symlinks?

@warrenfalk
Copy link
Contributor Author

Yes, exactly. Since symlinks are dereferenced, we will now see where the package actually is, and so we see workspace packages as outside of "node_modules" even though we found them through a symlink inside node_modules.

@nchanged nchanged merged commit 5dc3b3f into master Mar 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants