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.

Sign up for GitHub

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

Jump to bottom

Incompatible dependency: glob requires Node 20+ #508

Closed
bmish opened this issue Dec 5, 2024 · 12 comments
Closed

Incompatible dependency: glob requires Node 20+ #508

bmish opened this issue Dec 5, 2024 · 12 comments

Comments

@bmish
Copy link
Contributor

bmish commented Dec 5, 2024

markdownlint-cli depends on glob.

glob v11 requires Node 20+: https://github.com/isaacs/node-glob/blob/main/changelog.md#110

But markdownlint-cli currently specifies that it supports Node >= 18:

https://github.com/igorshubovych/markdownlint-cli/blob/0d9fcb51a54f3b750b911c054b4bd1a590f1b592/package.json#L10C16-L10C18

So the solution would be to downgrade glob until requiring Node 20+.
@bmish bmish mentioned this issue Dec 5, 2024
Merged
@DavidAnson
Copy link
Collaborator

True, but that was 6 months ago and this doesn't seem to have been a problem for CI or users. (I wish Dependabot wouldn't offer updates like this that are semi-incompatible.)

@DavidAnson
Copy link
Collaborator

@bmish, do you know of a tool that validates this (and could run in CI) or did you just happen to spot this manually?

@bmish bmish mentioned this issue Dec 5, 2024
Merged
@bmish
Copy link
Contributor Author

bmish commented Dec 5, 2024 •
edited
Loading

Here's a CI fix that would catch it:

I will note that this isn't really an urgent problem, so I don't mind if you want to:

  • Downgrade glob
  • Or just wait until 2025-04-30 to drop support for Node 18 and merge the CI fix so it doesn't happen again

@DavidAnson
Copy link
Collaborator

My current thinking is to wait, but leave the issue open for visibility. :)

@DavidAnson
Copy link
Collaborator

@bmish, FYI that markdownlint library and CLI2 now both have engine-strict enabled (without issue on Node 18) in their "next" branches.

@lsh-0
Copy link

lsh-0 commented Jan 8, 2025

My current thinking is to wait, but leave the issue open for visibility. :)

If we could get a 0.44.0 with a downgraded glob that would be helpful, thank you

@DavidAnson
Copy link
Collaborator

@lsh-0, versions 0.41.0 and earlier of markdownlint-cli use the older version of glob that declares support for Node 18. Node 20 and later are supported by glob with the latest version of markdownlint-cli. (And there have been no reported issues due to this in 6 months.) Is there a compelling case to downgrade?

@lsh-0
Copy link

lsh-0 commented Jan 8, 2025

Is there a compelling case to downgrade?

The set of dependencies described by markdownlint-lint are incompatible. It doesn't support node18 any more because of glob@11.x.

Downgrading glob seems the most natural fix but I guess you could also change this line and remove support for node18:

markdownlint-cli/package.json

Line 11 in e9435dd

"node": ">=18"

@DavidAnson
Copy link
Collaborator

I'm not familiar with markdownlint-lint, I don't see it on npm? But because there are two different ways to address this issue today (see my comment above) and no known problems with the current (inconsistent) configuration, how necessary is it to downgrade glob within this project?

@lsh-0
Copy link

lsh-0 commented Jan 8, 2025

I'm not familiar with markdownlint-lint, I don't see it on npm

Typo, I meant markdownlint-cli, this project.

and no known problems with the current (inconsistent) configuration, how necessary is it to downgrade glob within this project?

There are problems. We are having problems. There are two solutions or you can do nothing.

@DavidAnson
Copy link
Collaborator

Please give an example of a problem that is happening because of this situation along with the steps to reproduce it.

@bmish
Copy link
Contributor Author

bmish commented Jan 8, 2025

I would suggest just using an older version of markdownlint-cli until the problem is solved.

DavidAnson added a commit that referenced this issue Jan 20, 2025
@DavidAnson
Revert glob and minimatch dependencies to the most recent versions th…
4ebe776 
…at support Node 18 (still a valid "Maintenance LTS release") (fixes #508).
@samson-keung samson-keung mentioned this issue Feb 14, 2025
Merged
1 task
mergify bot pushed a commit to aws/aws-cdk that referenced this issue Feb 15, 2025
@samson-keung
chore: fix yarn upgrade causing dependency incompatibility (#33454)
0e67605 
### Issue # (if applicable)

N/A

The problem was `yarn upgrade` no longer worked. You can see the auto upgrade PR - #33299 - is having a failed build.

After diving deep into the reason of failure, here are the findings:

I first checked out the branch for #33299, then run the build locally. Here is the error in the build log:
```
> tsc --build

aws-cdk/node_modules/@types/glob/index.d.ts:29:42 - error TS2694: Namespace '"<path skipped>/aws-cdk/node_modules/minimatch/dist/commonjs/index"' has no exported member 'IOptions'.

29     interface IOptions extends minimatch.IOptions {
                                            ~~~~~~~~

aws-cdk/node_modules/@types/glob/index.d.ts:74:30 - error TS2724: '"<path skipped>/aws-cdk/node_modules/minimatch/dist/commonjs/index"' has no exported member named 'IMinimatch'. Did you mean 'Minimatch'?

74         minimatch: minimatch.IMinimatch;
                                ~~~~~~~~~~

```

Pay attention to the file paths above. `aws-cdk/node_modules/@types/glob` is trying to reference a type from `aws-cdk/node_modules/minimatch` because yarn upgraded to a `minimatch` version that natively export minimatch types. But `@types/glob` is not compatible with these new `minimatch` types, causing the error seen above.

Ideally, `@types/glob` should specify the `@types/minimatch` version it works with, but in reality, it has `"@types/minimatch": "*"`, which started pointing to the upgraded `aws-cdk/node_modules/minimatch` as yarn hoist dependencies into the top level `node_modules`.

Some references:
- igorshubovych/markdownlint-cli#508 <-- `aws-cdk/tools/@aws-cdk/cdk-build-tools` uses `markdownlint-cli`, which depend on `glob` and `minimatch` as well.
- isaacs/rimraf#264 <-- New versions of `glob` and `minimatch` are written in Typescript, which is causing problem when these new version co-exist with the `@types/xxx` packages.



### Description of changes

Use `nohoist` for `@types/glob` and `@types/minimatch` so that the different places that use these two packages do not conflict with each other at the top level `node_modules`.

After doing the above, I noticed `cdk-build-tools` was actually relying on `@types/glob` but it does not declare the dependency in its `package.json`. It worked because it pulled the `@types/glob` at the top level `node_modules` (which is no longer available with `nohoist`).

### Describe any new or updated permissions being added

None


### Description of how you validated changes

Locally built and no error.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
yashkh-amzn pushed a commit to yashkh-amzn/aws-cdk that referenced this issue Feb 21, 2025
@samson-keung @yashkh-amzn
chore: fix yarn upgrade causing dependency incompatibility (aws#33454)
0eb3022 
### Issue # (if applicable)

N/A

The problem was `yarn upgrade` no longer worked. You can see the auto upgrade PR - aws#33299 - is having a failed build.

After diving deep into the reason of failure, here are the findings:

I first checked out the branch for aws#33299, then run the build locally. Here is the error in the build log:
```
> tsc --build

aws-cdk/node_modules/@types/glob/index.d.ts:29:42 - error TS2694: Namespace '"<path skipped>/aws-cdk/node_modules/minimatch/dist/commonjs/index"' has no exported member 'IOptions'.

29     interface IOptions extends minimatch.IOptions {
                                            ~~~~~~~~

aws-cdk/node_modules/@types/glob/index.d.ts:74:30 - error TS2724: '"<path skipped>/aws-cdk/node_modules/minimatch/dist/commonjs/index"' has no exported member named 'IMinimatch'. Did you mean 'Minimatch'?

74         minimatch: minimatch.IMinimatch;
                                ~~~~~~~~~~

```

Pay attention to the file paths above. `aws-cdk/node_modules/@types/glob` is trying to reference a type from `aws-cdk/node_modules/minimatch` because yarn upgraded to a `minimatch` version that natively export minimatch types. But `@types/glob` is not compatible with these new `minimatch` types, causing the error seen above.

Ideally, `@types/glob` should specify the `@types/minimatch` version it works with, but in reality, it has `"@types/minimatch": "*"`, which started pointing to the upgraded `aws-cdk/node_modules/minimatch` as yarn hoist dependencies into the top level `node_modules`.

Some references:
- igorshubovych/markdownlint-cli#508 <-- `aws-cdk/tools/@aws-cdk/cdk-build-tools` uses `markdownlint-cli`, which depend on `glob` and `minimatch` as well.
- isaacs/rimraf#264 <-- New versions of `glob` and `minimatch` are written in Typescript, which is causing problem when these new version co-exist with the `@types/xxx` packages.



### Description of changes

Use `nohoist` for `@types/glob` and `@types/minimatch` so that the different places that use these two packages do not conflict with each other at the top level `node_modules`.

After doing the above, I noticed `cdk-build-tools` was actually relying on `@types/glob` but it does not declare the dependency in its `package.json`. It worked because it pulled the `@types/glob` at the top level `node_modules` (which is no longer available with `nohoist`).

### Describe any new or updated permissions being added

None


### Description of how you validated changes

Locally built and no error.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@benmccann benmccann mentioned this issue Feb 25, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@bmish @DavidAnson @lsh-0