Yarn workspace root path normalization

[nason]

Hi there,

Thanks for this handy module. I've been using it within a shared build tool with great effect. We're looking into moving into a Yarn Workspace setup and I noticed that this serializer does not handle normalization of the yarn workspace root path.

For example, in a non-workspace setup a path that was ~/code/single-project/node_modules/foo/foo.js would correctly normalize to <PROJECT_ROOT>/node_modules/foo/foo.js

However, in a Yarn workspace this may resolve to to ~/code/workspace/node_modules/foo/foo.js (the project in this case lives at ~/code/workspace/packages/single-project), which gets normalized as <HOME_DIR>/code/workspace/node_modules/foo/foo.js. Ideally, this would be relative to the workspace root (something like <WORKSPACE_ROOT>/node_modules/foo/foo.js) so that our tests are not dependent on a specific project location.

I took a very quick pass at this in https://github.com/tribou/jest-serializer-path/compare/master...nason:yarn-workspace-support?expand=1. It seems to be working for me, but wanted to get your thoughts on this before going any further.

[chrisblossom]

@nason good find! Although I think you should be running tests locally to each package, this is probably something that can be relatively easily supported for both yarn workspaces and lerna (even though exec and run already contains each package with the correct process.cwd root. I am unsure if yarn workspaces has a similar feature).

I think each workspace should use <PROJECT_ROOT> instead of <WORKSPACE_ROOT>. Tests shouldn't have to change if you pull a package out by itself.

Regarding the PR, tests will need to be added, which I don't mind helping with at all, as well as parse workspaceRootReal before workspaceRoot (you can see other tests to validate that).

I'm not sure about using find-yarn-workspace-root because I don't think it will work for Lerna as well, maybe I'm wrong?

Also, there can be multiple yarn workspaces within the same project. We'll need to handle that as well.

We'll probably want to add a separate test command that runs all existing tests inside a skeleton yarn workspace and Lerna environment.

Am I missing anything?

@tribou thoughts?

[tribou]

Yes, I agree this would be a great use case and support what @chrisblossom mentioned. I'm not well-versed with lerna or yarn workspaces, so just a couple questions/confirmations:

1. I think <PROJECT_ROOT> makes the most sense to cover individual packages, lerna repos, and yarn workspaces regardless of where the individual package lives.
2. If it helps to get this feature out faster, I'm fine with just focusing on yarn workspaces first and opening a separate ticket for lerna afterwards.
3. If lerna automatically contains the correct process.cwd, do we even need to do anything for lerna at this point? Maybe just adding a lerna test to be sure is a "nice-to-have" but not required for this change.
4. If 3 is correct, then perhaps find-yarn-workspace-root is the best option so long as it doesn't interfere with the lerna process.cwd. (again, lerna test could prove)
5. Skeleton yarn workspace and lerna environments in the test suite... 😞 It's more work than I'd like, but I agree and can't think of any good alternatives.

Feel free to provide any additional input, but it looks like these are the tasks needed to merge:

• [ ] Switch to using <PROJECT_ROOT> for a package root in yarn workspaces
• [ ] Split on the workspaceRootReal before the workspaceRoot
• [ ] @nason to submit a formal PR so @chrisblossom and myself can help out if needed
• [ ] Add a yarn workspaces fixture and test(s) (at the very least a snapshot test). Maybe borrow fixture ideas from the yarn test suite fixtures
• [ ] Add a lerna fixture and snapshot test borrowing ideas from the lerna test suite __fixtures__

[chrisblossom]

If it helps to get this feature out faster, I'm fine with just focusing on yarn workspaces first and opening a separate ticket for lerna afterwards.

This could mean rewriting the yarn workspaces PR. Not a deal though.

If lerna automatically contains the correct process.cwd, do we even need to do anything for lerna at this point? Maybe just adding a lerna test to be sure is a "nice-to-have" but not required for this change.

This is only if people use the lerna run and exec commands. I believe this is considered best practice, but in the majority of projects I've seen, they do not do this.

Also, it should be noted that lerna is significantly more popular than yarn workspaces, and that they are often used in combination with one another.

@nason What do you think of all that we've discussed so far?

[chrisblossom]

@nason If you don't have the time for this please let me know. I'll start working on it if not, no problem at all either way.

[tribou]

This is only if people use the lerna run and exec commands. I believe this is considered best practice, but in the majority of projects I've seen, they do not do this.

Unless we find a good, simple, side effect-free way to detect lerna packages when lerna run and lerna exec are not used, I would recommend we simply add a note in the README that lerna run and lerna exec should be used to run the Jest snapshot tests. (If users are running scripts in individual packages by switching process.cwd() to that package, then the snapshots should be fine anyway.)

@chrisblossom in the majority of projects you've seen where they don't use run and exec, are there any other valid use cases that would circumvent this detection?

[chrisblossom]

@tribou It is just as easy supporting lerna as it is yarn workspaces. IMO, they should be treated the same.

I'll try and put something together soon, although my free time is currently limited for a month or so.

[chrisblossom]

I'm now thinking it is better to not handle yarn workspaces and lerna as special cases. Otherwise it will make relative paths inconsistent to absolute paths. This most likely applies to https://github.com/tribou/jest-serializer-path/pull/32 as well.

For example:

const path = require('path');

const file = 'packages/test-pkg/src/index.js';

const absolutePath = path.resolve(process.cwd(), file); // <PROJECT_ROOT>/src/index.js
const relativePath = path.relative(process.cwd(), file); // packages/test-pkg/src/index.js

[nason]

Hey @chrisblossom, I'm not 100% sure I'll have time this week but I will do my best to get on this and will leave any updates I have here.

I have snapshot tests disabled in my project until this is resolved so I'd really love to land a fix here 🎈

---

I'm now thinking it is better to not handle yarn workspaces and lerna as special cases. Otherwise it will make relative paths inconsistent to absolute paths.

Can you explain what you mean? I'm not sure I follow. Let me try to rephrase the issue I was seeing:

Before moving to a workspace, this serializer made all paths relative to the project root so that snapshots were nicely portable.

After moving to a workspace, the serializer does not seem to detecting a project root (or a workspace root, as suggested above) and outputs paths relative to my home directory, resulting in non-portable snapshots.

[chrisblossom]

@nason I am unsure why you are seeing <HOME_DIR> instead of <PROJECT_ROOT>. This sounds like a bug, not an issue with yarn workspaces.

I've created a minimal repo that showcases lerna, yarn workspaces and lerna + yarn workspaces. https://github.com/chrisblossom/jest-serializer-path-issue-30

Please send a PR to that repo or create a minimal repository to show what you are seeing.

Can you explain what you mean? I'm not sure I follow. Let me try to rephrase the issue I was seeing:

My code block in https://github.com/tribou/jest-serializer-path/issues/30#issuecomment-390439577 shows the comments of the result. You can see relative paths will be different than absolute paths, which I do not think this is expected / wanted.

Another issue with directly supporting yarn workspaces / lerna, is where node_modules are located. Because they are typically outside of the package root, it will give unexpected results for snapshots that have node_modules.

Lots of random edge cases here.

BTW, <PROJECT_ROOT> === process.cwd()