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

chore: change workspace packages pattern #135

Merged
merged 1 commit into from
Jan 3, 2020

Conversation

Andarist
Copy link
Member

@Andarist Andarist commented Jan 2, 2020

Description

I've noticed that the currently used pattern was overly greedy and has classified such things as potential packages:

packages/auto-install/test/fixtures/npm
packages/auto-install/test/fixtures/yarn
packages/auto-install/test/fixtures/yarn-bare
packages/json/test/fixtures/named
packages/node-resolve/test/fixtures

Things were working, but including those as workspace packages was not intentional so better to fix that.

@@ -68,12 +68,6 @@ importers:
node-noop: ^1.0.0
rollup: ^1.20.0
rollup-plugin-node-resolve: ^5.2.0
packages/auto-install/test/fixtures/npm:
Copy link
Member Author

Choose a reason for hiding this comment

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

you can actually see how those were incorrectly included here

@@ -1,8 +1,8 @@
importers:
Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea why there are so many lockfile changes. I've just executed pnpm i after fixing that workspace option - which had to be done, because it affects lockfile. Not sure why any versions here were changed though.

Copy link
Collaborator

@shellscape shellscape left a comment

Choose a reason for hiding this comment

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

Good catch 👍

@Andarist
Copy link
Member Author

Andarist commented Jan 2, 2020

A little strange situation - some snapshots have failed due to end of line difference (classic crlf vs lf). It works on master though and I haven't touched those files as part of this PR. Any ideas? My guess is that some dependency has changed but I don't know why they have changed in the first place.

@shellscape
Copy link
Collaborator

When you changed the scope and removed the (incorrectly) included packages, pnpm recognized that and cleaned up the lockfile. I don't know why your changes are now causing issues in snapshots, because that doesn't happen on other branches.

@Andarist Andarist force-pushed the change-workspace-pkgs-pattern branch from 59e5d0c to 92d5294 Compare January 2, 2020 23:35
@Andarist Andarist force-pushed the change-workspace-pkgs-pattern branch from 92d5294 to b0f0e8f Compare January 2, 2020 23:42
@Andarist
Copy link
Member Author

Andarist commented Jan 2, 2020

I've "fixed" the problem by normalizing yarn-written files on our end (maybe yarn version has changed on CI in the meantime? although can't find any changelog entry in yarn about a change that could affect this).

@shellscape
Copy link
Collaborator

This goes back to #69 and your comment about mocking yarn out. I'm OK if we want to put that off to another PR, but let's put a comment above the changed test lines to explain why that's there. Otherwise it's going to look superfluous to someone not aware of this PR.

@shellscape shellscape merged commit 6f1b9f1 into master Jan 3, 2020
@Andarist Andarist deleted the change-workspace-pkgs-pattern branch January 3, 2020 10:18
LarsDenBakker pushed a commit to LarsDenBakker/plugins that referenced this pull request Sep 12, 2020
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.

2 participants