Skip to content

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)

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants