Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: throw on bad version with correct error message #552

Merged
merged 1 commit into from
Apr 17, 2023

Conversation

wraithgar
Copy link
Member

@wraithgar wraithgar commented Apr 14, 2023

The parsing logic that is duplicated in SemVer's constructor was also removed. See #541 for previous discussions about not optimizing for invalid SemVer.

With this fix:

> semver.diff('foo', '1.2.3')
Uncaught TypeError: Invalid Version: foo

v7.4.0:

> semver.diff('foo', '1.2.3')
Uncaught TypeError: Invalid Version: null

503a4e52:

> semver.diff('foo', '1.2.3')
Uncaught TypeError: Cannot read properties of null (reading 'compare')

@wraithgar wraithgar requested a review from a team as a code owner April 14, 2023 16:10
@wraithgar wraithgar requested a review from nlf April 14, 2023 16:10
@wraithgar wraithgar changed the title fix: throw on bad version fix: throw on bad version with correct error message Apr 14, 2023
@wraithgar
Copy link
Member Author

wraithgar commented Apr 14, 2023

This does reinstantiate, perhaps we need an unsafe parse (i.e. one that doesn't catch)

functions/parse.js Outdated Show resolved Hide resolved
@wraithgar wraithgar force-pushed the gar/parse-early branch 2 times, most recently from c6b0ce1 to e0d7c01 Compare April 15, 2023 00:04
@wraithgar
Copy link
Member Author

Arrays and objects still throw an ambiguous message but is it worth bringing in something like util.inspect?

> require('.').diff([], '1.3.2')
Uncaught TypeError: Invalid Version: 
> require('.').diff({}, '1.3.2')
Uncaught TypeError: Invalid Version: [object Object]

vs

throw new TypeError(`Invalid Version: ${require('util').inspect(version)}`)`
> require('.').diff([], '1.3.2')
Uncaught TypeError: Invalid Version: []
> require('.').diff({}, '1.3.2')
Uncaught TypeError: Invalid Version: {}

@tjenkinson
Copy link
Contributor

Given that code path should only be hit if you're doing something really wrong, I don't see any harm in making the error more readable with inspect

@wraithgar wraithgar merged commit e219bb4 into main Apr 17, 2023
@wraithgar wraithgar deleted the gar/parse-early branch April 17, 2023 17:00
@UNIDY2002
Copy link

UNIDY2002 commented Apr 18, 2023

I don't see any harm in making the error more readable with inspect

Oops... This does break something.

I am using this library in a react-native project, and since RN does not run in a Node.js environment, its bundler might not recognize the "util" module, causing bundle failure.

I've just received a failure report, and haven't investigated into it.


Update: I installed util for polyfill, and now it is working fine.

@tjenkinson
Copy link
Contributor

Ah I checked the node version support of it but didn’t realise this was also used in a browser 🙈

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants