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

[Deps] update contains-path, doctrine, pkg-up, read-pkg-up #2036

Merged
merged 1 commit into from
May 12, 2021

Conversation

Semigradsky
Copy link
Contributor

No description provided.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

These are all breaking changes, and almost all of them drop node versions, which means this plugin can effectively never update to them.

Why should these be updated - what motivates this PR?

@Semigradsky
Copy link
Contributor Author

Maybe I missed something. As I see all these versions still support node.js 4.

@ljharb
Copy link
Member

ljharb commented Apr 26, 2021

Yes, and we support node 0.10 and above (via eslint 2), so that would be a breaking change.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.9%) to 71.388% when pulling da2ab1c on Semigradsky:master into e871a9a on benmosher:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+1.9%) to 71.388% when pulling da2ab1c on Semigradsky:master into e871a9a on benmosher:master.

@coveralls
Copy link

coveralls commented Apr 26, 2021

Coverage Status

Coverage increased (+14.4%) to 84.048% when pulling 5898e28 on Semigradsky:master into 17a445d on benmosher:master.

@Semigradsky
Copy link
Contributor Author

Sorry, I don't know about this... Shouldn't there be 0.10 in engines field in package.json?

@ljharb
Copy link
Member

ljharb commented Apr 26, 2021

There could be, but there shouldn't be because then our package would be claiming compatibility with node 0.10 even if the user uses eslint 7, for example. In this case, the engines requirements of our peer deps apply to us, but must be implicit.

@Semigradsky
Copy link
Contributor Author

Eslint 7 officially doesn't support node 4, 6, even 8, but you do. Is this not the same case?

@ljharb
Copy link
Member

ljharb commented Apr 26, 2021

Yes - if we put node 4 in out "engines" field, then it would be inaccurate when eslint 7 is installed, but it is accurate with older eslint versions. Thus, we don't have an "engines" field, but we still need to remain compatible with eslint 2 - 7 (and all the node versions those support).

@Semigradsky
Copy link
Contributor Author

Maybe I smth don't understand but it looks the same for me.

If we put node 0.10 in "engines" field, then it would be inaccurate when eslint 7 is installed, but it is accurate with older eslint versions (eslint 2).

But there is ">=4" in engines:
https://github.com/benmosher/eslint-plugin-import/blob/e871a9a2e18fd4d67a5a237789131b44cc705b5b/package.json#L6

@ljharb
Copy link
Member

ljharb commented Apr 26, 2021

ha, fair point, i didn't realize we'd already required that.

In that case, perhaps we could update to these versions - can you confirm that you've checked the changelogs for each, and that the only breaking changes are dropping node < 4 support?

@Semigradsky
Copy link
Contributor Author

Yep.

  • "contains-path": "^1.0.0" - "node": ">=0.10.0"
  • "doctrine": "^2.1.0" - "node": ">=0.10.0"
  • "is-core-module": "^2.3.0" - no engines field, but no specific in code and there is only one dependency which has "node": ">= 0.4.0"
  • "pkg-up": "^2.0.0" - "node": ">=4"
  • "read-pkg-up": "^3.0.0" - "node": ">=4"

@ljharb ljharb self-assigned this Apr 26, 2021
@ljharb
Copy link
Member

ljharb commented Apr 26, 2021

Master's currently broken (see #1986), so this will have to wait until that's resolved.

@ljharb ljharb changed the title [Deps] Update contains-path, doctrine, is-core-module, pkg-up, read-pkg-up [Deps] update contains-path, doctrine, pkg-up, read-pkg-up May 12, 2021
@ljharb ljharb merged commit 5898e28 into import-js:master May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants