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

feat(config): allow to provide glob expressions for linked and ignore packages #458

Merged
merged 7 commits into from
Oct 6, 2020

Conversation

emmenko
Copy link
Contributor

@emmenko emmenko commented Sep 17, 2020

Closes #364

@changeset-bot
Copy link

changeset-bot bot commented Sep 17, 2020

🦋 Changeset detected

Latest commit: 367589c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@changesets/config Minor
@changesets/cli Minor

Not sure what this means? Click here to learn what changesets are.

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

@atlassian-cla-bot
Copy link

atlassian-cla-bot bot commented Sep 17, 2020

Hooray! All contributors have signed the CLA.

@@ -17,6 +18,14 @@ let defaultPackages: Packages = {
tool: "yarn"
};

const withPackages = (pkgNames: string[]) => ({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Helper function for the tests

Comment on lines 34 to 53
function normalizePackageNames(
listOfPackageNamesOrGlob: readonly string[],
pkgNames: readonly string[]
): string[] {
// Resolve and expand possible glob esxpressions.
return listOfPackageNamesOrGlob.reduce<string[]>(
(allResolvedPackageNames, pkgNameOrGlob) => {
const matchingPackages = pkgNames.filter(minimatch.filter(pkgNameOrGlob));
return [
...allResolvedPackageNames,
// If there are no matching packages as a result of the minimatch filter,
// it is most likely the case when a package defined in the linked config
// does not exist.
// To be able to show a correct validation message, we pass the value as-is.
...(matchingPackages.length > 0 ? matchingPackages : [pkgNameOrGlob])
];
},
[]
);
}
Copy link
Contributor Author

@emmenko emmenko Sep 17, 2020

Choose a reason for hiding this comment

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

This is the main function to resolve the globs. It uses minimatch.filter (https://www.npmjs.com/package/minimatch#minimatchfilterpattern-options) micromatch to resolve the package name and spread it to the list.

Comment on lines 146 to 149
let normalizedLinkedGroup = normalizePackageNames(
linkedGroup,
pkgNames
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We normalize the list before doing the validation. This allows the retain the same logic as before.

);
for (let pkgName of json.ignore) {
if (!pkgNames.has(pkgName)) {
let normalizedIgnore = normalizePackageNames(json.ignore, pkgNames);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought we can allow globs also for the ignore list.

@emmatown
Copy link
Member

@Noviny @Andarist what are your opinions on globs vs regex for this?

My thoughts:

globs:

  • the inconsistency of what a "glob" is the worst
  • they're kinda easier to write than regex, especially when in a string literal
  • they're what people are used to writing in most tools(e.g. workspaces config)
  • all valid npm packages names will only ever match that package name

regex:

  • regex is consistent(or rather consistent in JS)
  • The whole double escaping thing would get ANNOYING
  • v important point: A valid npm package name could match something that isn't the package name(e.g. you have a package named something.another.thing, will match something-something.another.thing-whatever-else, something1another1thing and somethingaanotherathing and etc. To make it only match something.another.thing, you would have to write "^something\\.another\\.thing$"). This would presumably mean we would need a syntax for "I want to do regex, not exact package name matching", probs /regex here/? so it would be "/^something\\.another\\.thing$/"
  • You'd probably always want to write ^my-package$ rather than my-package so that it doesn't match my-package-whatever which is just ughhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhh

I feel like I'm leaning towards globs but wouldn't mind hearing other people's thoughts

@@ -16,7 +16,8 @@
"@changesets/logger": "^0.0.5",
"@changesets/types": "^3.1.0",
"@manypkg/get-packages": "^1.0.1",
"fs-extra": "^7.0.1"
"fs-extra": "^7.0.1",
"minimatch": "^3.0.4"
Copy link
Member

Choose a reason for hiding this comment

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

Can you use micromatch instead? (micromatch is what fast-glob uses which globby uses which is what @manypkg/get-packages uses which is what changesets uses for getting the packages in a monorepo so it's good to use the same thing for consistency and not spending the time loading two glob implementations when people use Changesets)

(this comment is assuming we go with globs)

Copy link
Contributor Author

@emmenko emmenko Sep 17, 2020

Choose a reason for hiding this comment

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

Damn, I thought I read "minimatch" from your comment, but it was "micromatch" instead. 🤦‍♂️
My bad sorry, I'll change that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 2f0af89

packages/config/src/index.test.ts Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 17, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@a60c0e1). Click here to learn what that means.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #458   +/-   ##
=========================================
  Coverage          ?   77.64%           
=========================================
  Files             ?       42           
  Lines             ?     1275           
  Branches          ?      308           
=========================================
  Hits              ?      990           
  Misses            ?      275           
  Partials          ?       10           
Impacted Files Coverage Δ
packages/config/src/index.ts 96.80% <100.00%> (ø)
...y-release-plan/src/test-utils/failing-functions.ts 50.00% <0.00%> (ø)
...s/get-dependents-graph/src/get-dependency-graph.ts 100.00% <0.00%> (ø)
packages/cli/src/utils/isCI.ts 0.00% <0.00%> (ø)
packages/cli/src/commands/add/createChangeset.ts 54.65% <0.00%> (ø)
packages/logger/src/index.ts 100.00% <0.00%> (ø)
packages/assemble-release-plan/src/apply-links.ts 90.32% <0.00%> (ø)
packages/apply-release-plan/src/utils.ts 91.66% <0.00%> (ø)
packages/cli/src/commands/pre/index.ts 89.47% <0.00%> (ø)
packages/apply-release-plan/src/index.ts 92.63% <0.00%> (ø)
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a60c0e1...367589c. Read the comment docs.

return listOfPackageNamesOrGlob.reduce<string[]>(
(allResolvedPackageNames, pkgNameOrGlob) => {
const matchingPackages = pkgNames.filter(pkgName =>
micromatch.isMatch(pkgName, pkgNameOrGlob)
Copy link
Member

@emmatown emmatown Sep 20, 2020

Choose a reason for hiding this comment

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

I don't this would work for configs like ["pkg-*", "!pkg-a"]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked into that. It's a bit tricky and can become ambiguous and a bit confusing to what the expectation results are.

For example, given these packages:

pkg-a
pkg-b
@pkg/a
@pkg/b
@pkg-other/a
@pkg-other/b

and this linked config:

{
  linked: [["pkg-*", "!pkg-b", "@pkg/*"]]
}

When I read this as a user, I can infer that I want all packages that start with pkg- but not pkg-b, plus all packages that start with @pkg/.
However, evaluating !pkg-b by itself basically means "all packages that are not pkg-b", which technically would mean to include some packages that weren't meant to be included like @pkg-other/a and @pkg-other/b.
Also the order of the linked packages might be important. For instance ["pkg-*", "!pkg-b"] could be interpreted differently than ["!pkg-b", "pkg-*"].

I guess we could find and settle on a reasonable behavior but I fear that this might cause more confusion than ever.


FYI: with the current implementation, using a config like

linked: [["pkg-*", "!pkg-b", "@pkg/*"], ["@pkg-other/a"]]

yields some validation errors:

ValidationError: Some errors occurred when validating the changesets config:
    The package "pkg-a" is defined in multiple sets of linked packages. Packages can only be defined in a single set of linked packages. If you are using glob expressions, make sure that they are valid according to https://www.npmjs.com/package/micromatch.
    The package "@pkg/a" is defined in multiple sets of linked packages. Packages can only be defined in a single set of linked packages. If you are using glob expressions, make sure that they are valid according to https://www.npmjs.com/package/micromatch.
    The package "@pkg/b" is defined in multiple sets of linked packages. Packages can only be defined in a single set of linked packages. If you are using glob expressions, make sure that they are valid according to https://www.npmjs.com/package/micromatch.
    The package "@pkg-other/a" is defined in multiple sets of linked packages. Packages can only be defined in a single set of linked packages. If you are using glob expressions, make sure that they are valid according to https://www.npmjs.com/package/micromatch.

Copy link
Member

Choose a reason for hiding this comment

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

How such groups with multiple patterns are handled by other tools? Could we take a look at prior work done by others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't think of other examples of this, besides .gitignore and such.

Maybe we should be more strict and allow either/or and not a mix. If you provide a list, it should be a list of package names. No globs or regex.
If you want to provide a glob or regex, you provide it as a single value, not a list.

For example:

linked: [["pkg-a", "pkg-c", "@pkg/a", "@pkg/b"]]

is equivalent of

linked: [/(pkg\-[^b])|(@pkg\/\w)/]

image

Building regular expressions can be tricky though, especially for advanced used cases.


Another option I can think of is to provide a named configuration object where you explicitly define which packages should be included and which should be excluded.
Then you can use globs again.

For example:

linked: [["pkg-a", "pkg-c", "@pkg/a", "@pkg/b"]]

is equivalent of

linked: [
  {
    include: ['pkg-*', '@pkg/*'],
    exclude: ['pkg-b', '@pkg-others/*']
  }
]

The exclude list will always be resolved and applied lastly.

Copy link
Member

Choose a reason for hiding this comment

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

One place that comes to my mind that we could look into how they have solved are workspace settings in yarn/lerna. Those are glob-based as well

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what the issue here is, the following is what micromatch does and is what we want:

micromatch(
  ["pkg-a", "pkg-b", "pkg-c", "@pkg/a", "@pkg/b", "@pkg/c", "something"],
  ["pkg-*", "!pkg-b", "@pkg/*"]
)
// ["pkg-a", "pkg-c", "@pkg/a", "@pkg/b", "@pkg/c"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mitchellhamilton thanks, yes you're right. We can simply do it like that, I think I got confused with the switch from minimatch to micromatch. My bad 🤦‍♂️

I'll push a commit with the fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mitchellhamilton @Andarist I pushed a commit with the simplified matching logic. b2af53d

Note that for keeping the current validation logic, we're returning a tuple from the normalizePackageNames function. The second argument is a list of packages that didn't match.
As a follow up, as suggested here #458 (comment), we can look into refactoring the validation logic.

Copy link
Member

Choose a reason for hiding this comment

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

From what I see in Lerna's code this doesn't conform to their logic for resolving packages, see code pointers in this post: #458 (comment) . Nor it does conform to the same thing in yarn: https://github.com/yarnpkg/yarn/blob/cbeead63564f1e1a9bdebbfec7f1fafde04f8c91/src/config.js#L819-L826

Are we fine with this?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is fine IMO

@Noviny
Copy link
Collaborator

Noviny commented Sep 21, 2020

I think I'd favour globs

@Andarist
Copy link
Member

While I hate globs and I've vented about it in another thread - I think they make more sense here, especially given serializability issues.

@taras
Copy link

taras commented Oct 5, 2020

👋 how is this going? Can I help?

@emmenko
Copy link
Contributor Author

emmenko commented Oct 5, 2020

I think we're waiting for a decision around the behavior of inclusive/exclusive patterns.

@Andarist @mitchellhamilton @Noviny any update from your side?

docs/config-file-options.md Show resolved Hide resolved
packages/config/package.json Outdated Show resolved Hide resolved
packages/config/src/index.test.ts Outdated Show resolved Hide resolved
packages/config/src/index.test.ts Show resolved Hide resolved
packages/config/src/index.ts Outdated Show resolved Hide resolved
packages/config/src/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

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

Thanks!!

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.

Allow to pass glob or regex to linked config
5 participants