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

fix: show type on invalid semver error #559

Merged
merged 8 commits into from
May 12, 2023

Conversation

tjenkinson
Copy link
Contributor

@tjenkinson tjenkinson commented May 5, 2023

Based on this comment from @wraithgar

If we want to revert this, it needs to be done in a way that also sets up some sort of linting or other testing so that "does not use x internals from node" is part of the contract, which it wasn't before.

This configures some eslint plugins to prevent importing node builtins or dev dependencies.

It also removes the dependency on node util and updates the error message slightly.

References

Fixes #554

.gitignore Outdated Show resolved Hide resolved
@tjenkinson tjenkinson marked this pull request as ready for review May 5, 2023 13:48
@tjenkinson tjenkinson requested a review from a team as a code owner May 5, 2023 13:48
@wraithgar
Copy link
Member

Is there really no eslint rule for this? I find that amazing if so.

@tjenkinson
Copy link
Contributor Author

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

classes/semver.js Outdated Show resolved Hide resolved
@wraithgar
Copy link
Member

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?

@wraithgar
Copy link
Member

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 file.

module.exports = {
plugins: ['import'],
rules: {
'import/no-extraneous-dependencies': [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this may not be required. checking

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yep. I think this config is useful though? The default would allow importing from devDependencies

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. done!

@tjenkinson tjenkinson force-pushed the ensure-no-externals branch 5 times, most recently from b185f4d to 199419d Compare May 5, 2023 14:23
@tjenkinson
Copy link
Contributor Author

tjenkinson commented May 5, 2023

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?

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 tjenkinson changed the title Check that no externals/builtins are used, and remove dependency on node util fix: check that no externals/builtins are used, and remove dependency on node util May 5, 2023
@tjenkinson tjenkinson changed the title fix: check that no externals/builtins are used, and remove dependency on node util fix: check no externals/builtins are used, and remove dependency on node util May 5, 2023
test/classes/semver.js Outdated Show resolved Hide resolved
Copy link

@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.

An alternative would be to use the "browser" field to specify an alternative for util.inspect outside of node, which could be typeof, but then node usage would still get the nice util.inspect experience.

@tjenkinson
Copy link
Contributor Author

Yeh I'm not sure if the benefit of inspect is really worth it for this case though?

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.

[BUG] Webpack builds are failing as new release 7.5.0 requiers node.js util
4 participants