fix: show type on invalid semver error #559

tjenkinson · 2023-05-05T13:45:52Z

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

wraithgar · 2023-05-05T13:50:15Z

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

tjenkinson · 2023-05-05T13:54:00Z

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

wraithgar · 2023-05-05T13:57:01Z

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 · 2023-05-05T14:00:10Z

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.

tjenkinson · 2023-05-05T14:17:25Z

.eslintrc.local.js

+module.exports = {
+  plugins: ['import'],
+  rules: {
+    'import/no-extraneous-dependencies': [


this may not be required. checking

It's already in the base ruleset https://github.com/npm/eslint-config/blob/9e87b58d9e049004c802239cab1fabbca67caf74/lib/index.js#L213

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

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.

cf https://github.com/npm/cli/blob/3d5bbcc86c484ef0b6c04ee0236c1072d36915d6/.eslintrc.local.json#L3 or https://github.com/npm/cacache/blob/de12400af2ab452f1e75a15edaf7ea51b294667b/.eslintrc.local.js#L4

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.

Opened npm/template-oss#316

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

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 · 2023-05-05T14:41:26Z

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.

… base config

test/classes/semver.js

ljharb

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 · 2023-05-05T17:01:11Z

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

ljharb · 2023-05-05T17:10:23Z

¯\_(ツ)_/¯ ime the typeof the value isn't very useful when debugging a failure, but the inspect output is very useful.

## References Refs npm/template-oss#316 Needed by npm/node-semver#559

…se config

wraithgar · 2023-05-12T15:47:41Z

Thanks @tjenkinson this was quite a journey. Thanks also for swimming all the way upstream into eslint-config. I'm looking forward to updating template-oss with that new version. Managing 80+ repos takes quite a few hard lines in the sand, and keeping our linting centrally managed is one of them.

mbtools · 2023-06-25T14:50:39Z

classes/semver.js

@@ -16,7 +16,7 @@ class SemVer {
        version = version.version
      }
    } else if (typeof version !== 'string') {
-      throw new TypeError(`Invalid Version: ${require('util').inspect(version)}`)
+      throw new TypeError(`Invalid version. Must be a string. Got type "${typeof version}".`)


"must be a string or a SemVer" would be more accurate

tjenkinson commented May 5, 2023

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

tjenkinson force-pushed the ensure-no-externals branch from 24e6cf2 to e18d94d Compare May 5, 2023 13:47

tjenkinson added 2 commits May 5, 2023 14:47

chore: add test step that makes sure no external/builtin dependencies

7328e66

fix: remove dependency on node builtin

d0a105e

tjenkinson force-pushed the ensure-no-externals branch from e18d94d to d0a105e Compare May 5, 2023 13:48

tjenkinson marked this pull request as ready for review May 5, 2023 13:48

tjenkinson requested a review from a team as a code owner May 5, 2023 13:48

tjenkinson mentioned this pull request May 5, 2023

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

Closed

1 task

wraithgar reviewed May 5, 2023

View reviewed changes

classes/semver.js Outdated Show resolved Hide resolved

tjenkinson commented May 5, 2023

View reviewed changes

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

tjenkinson added 2 commits May 5, 2023 15:38

chore: use eslint plugin for preventing externals/builtins

3cfdf1e

fix: tweak error message when input is wrong type

5019bc5

tjenkinson force-pushed the ensure-no-externals branch from 199419d to 5019bc5 Compare May 5, 2023 14:39

fix: don't specify eslint plugin in config because already there from…

7acab34

… base config

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

ljharb reviewed May 5, 2023

View reviewed changes

test/classes/semver.js Outdated Show resolved Hide resolved

chore: undo unneeded formatting changes

6f3bfbf

ljharb reviewed May 5, 2023

View reviewed changes

This was referenced May 6, 2023

fix: do not allow importing dev dependencies from src npm/template-oss#316

Closed

fix: do not allow importing dev deps from lib/bin npm/eslint-config#67

Merged

wraithgar pushed a commit to npm/eslint-config that referenced this pull request May 9, 2023

fix: do not allow importing dev deps from lib/bin (#67)

653976f

## References Refs npm/template-oss#316 Needed by npm/node-semver#559

tjenkinson added 2 commits May 12, 2023 10:12

chore: only add the devDependencies option to be consistent with ba…

9a3bd04

…se config

target specific dirs

b453496

wraithgar changed the title ~~fix: check no externals/builtins are used, and remove dependency on node util~~ fix: show type on invalid semver error May 12, 2023

wraithgar self-assigned this May 12, 2023

wraithgar approved these changes May 12, 2023

View reviewed changes

wraithgar merged commit d30d25a into npm:main May 12, 2023
24 checks passed

github-actions bot mentioned this pull request May 12, 2023

chore: release 7.5.1 #560

Merged

ms-bot mentioned this pull request Jun 5, 2023

chore(deps): bump 🧳 semver to ^7.6.0 mobsuccess-devops/pmd-github-action#11

Draft

mbtools reviewed Jun 25, 2023

View reviewed changes

Gornator mentioned this pull request Jul 2, 2023

[BUG] <title> #581

Closed

1 task

This was referenced Jul 31, 2023

Bump semver from 7.3.5 to 7.5.2 in /platform/ui ossrs/oryx#107

Closed

Bump semver from 7.3.5 to 7.5.4 in /platform/ui ossrs/oryx#111

Closed

Bump semver from 7.3.5 to 7.5.2 in /ui ossrs/oryx#116

Closed

Pezmc mentioned this pull request Sep 11, 2023

Update semver to avoid CVE FlowFuse/flowfuse#2718

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: show type on invalid semver error #559

fix: show type on invalid semver error #559

tjenkinson commented May 5, 2023 •

edited

wraithgar commented May 5, 2023

tjenkinson commented May 5, 2023

wraithgar commented May 5, 2023

wraithgar commented May 5, 2023

tjenkinson May 5, 2023

wraithgar May 5, 2023

tjenkinson May 5, 2023

wraithgar May 5, 2023

wraithgar May 5, 2023

wraithgar May 5, 2023

tjenkinson May 6, 2023

tjenkinson May 12, 2023

wraithgar May 12, 2023

tjenkinson May 12, 2023

tjenkinson commented May 5, 2023 •

edited

ljharb left a comment

tjenkinson commented May 5, 2023

ljharb commented May 5, 2023

wraithgar commented May 12, 2023

mbtools Jun 25, 2023

fix: show type on invalid semver error #559

fix: show type on invalid semver error #559

Conversation

tjenkinson commented May 5, 2023 • edited

References

wraithgar commented May 5, 2023

tjenkinson commented May 5, 2023

wraithgar commented May 5, 2023

wraithgar commented May 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjenkinson commented May 5, 2023 • edited

ljharb left a comment

Choose a reason for hiding this comment

tjenkinson commented May 5, 2023

ljharb commented May 5, 2023

wraithgar commented May 12, 2023

Choose a reason for hiding this comment

tjenkinson commented May 5, 2023 •

edited

tjenkinson commented May 5, 2023 •

edited