fix: show type on invalid semver error #559
Conversation
tjenkinson
force-pushed
the
ensure-no-externals
branch
from
24e6cf2
to
e18d94d
Compare
tjenkinson
force-pushed
the
ensure-no-externals
branch
from
e18d94d
to
d0a105e
Compare
|
Is there really no eslint rule for this? I find that amazing if so. |
|
Hmm actually didn't think about that. https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-extraneous-dependencies.md looks like it may work will give it a go |
|
Is there something special about rollup or is it a coincidence that it also only supports the same subset of node internals that webpack does? |
|
https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-nodejs-modules.md Is probably our rule. You'll need to override it in tests and map.js. eslint rules can be extended by adding a |
.eslintrc.local.js
Outdated
| module.exports = { | ||
| plugins: ['import'], | ||
| rules: { | ||
| 'import/no-extraneous-dependencies': [ |
There was a problem hiding this comment.
this may not be required. checking
There was a problem hiding this comment.
It's already in the base ruleset https://github.com/npm/eslint-config/blob/9e87b58d9e049004c802239cab1fabbca67caf74/lib/index.js#L213
There was a problem hiding this comment.
Ah yep. I think this config is useful though? The default would allow importing from devDependencies
There was a problem hiding this comment.
The file is already imported as a plugin, so the extra dependency isn't needed. I think a single new rule is all we need here with an allow of ./test and map.js.
There was a problem hiding this comment.
Then this needs to be addressed in template-oss, that's not a rule that we'd want to only apply to a single one of our over 80 repos.
There was a problem hiding this comment.
Opened npm/template-oss#316
There was a problem hiding this comment.
@wraithgar so given this doesn't follow the standard structure npm/eslint-config#67 doesn't have any effect here, so is the custom config in this PR good to go?
There was a problem hiding this comment.
Yeah we'll need a custom config but this one looks to be a bit different in approach than the one we landed on in eslint-config. We should pattern it after the one there but instead of lib and bin it should be bin, classes, functions, internal, and ranges (and now we begin to see why we standardized on lib).
The way this PR currently does it is by completely bypassing the no-extraneous-dependencies rule for the test files, which is a regression.
tjenkinson
force-pushed
the
ensure-no-externals
branch
5 times, most recently
from
b185f4d
to
199419d
Compare
tjenkinson
force-pushed
the
ensure-no-externals
branch
from
199419d
to
5019bc5
Compare
By default rollup doesn't understand any node_module dependencies. There is a node-resolve plugin that adds that support for those to be found. It also doesn't pollyfill any node builtins out of the box. |
tjenkinson
changed the title
utilutil
tjenkinson
changed the title
utilutil
|
Yeh I'm not sure if the benefit of |
Based on this comment from @wraithgar
This configures some eslint plugins to prevent importing node builtins or dev dependencies.
It also removes the dependency on node
utiland updates the error message slightly.References
Fixes #554