Update semver.js

[jasonkuhrt]

Fix error that is at least occurring in webpack compilation. References webpack/webpack#828.

Tasks prior to merge:

• Update AMD test.
• Answer question regarding whether AMD support should be outright removed.

[isaacs]

Please also update the amd test, and give it a proper commit message.

[jasonkuhrt]

@isaacs Ok will do, I wanted to first vet this change generally. I will follow up with a test and such given it seems to be ok. Thanks.

[isaacs]

Seems fine to me. I would also be ok with just removing the amd stuff entirely from this module. It's a bit odd, and most libs don't include it directly, so there's gotta be some kind of general purpose "amdify this commonjs module" thing, right?

[jasonkuhrt]

Seems fine to me. I would also be ok with just removing the amd stuff entirely from this module. It's a bit odd, and most libs don't include it directly, so there's gotta be some kind of general purpose "amdify this commonjs module" thing, right?

Honestly I can't say, I've never used AMD in my life, which I guess adds credence to your point but I'm just not sure about the "auto-amd" tooling out there.

@jhnns You probably know better. Ramifications of removing hard-coded AMD support?

[jhnns]

Sorry, I don't use AMD too. I'd remove it entirely but there are probably some AMD folks which will be mad then 😁.

If we don't want to remove AMD, we probably should use UMD instead of the current approach.

[tgriesser]

👍 to at least the patch, if not removing AMD entirely

[jasonkuhrt]

This issue is super low priority for me right now sorry. I'm more than happy to pass the baton to someone else. Takers?

[tgriesser]

What all needs to be done, is this a thing that needs a test?

It feels more along the lines of fixing a syntax error than a feature change.

[isaacs]

@tgriesser yes, every change needs a test. And a change that breaks a test is sadly something we can't accept. (This change breaks a test.)

Again, I'd really like to just remove AMD entirely from this module. It's something that belongs in the builder, not this module, imo. The next major version release will remove it entirely.

[mistadikay]

+1, having this error as well. So I guess there is nothing to do, but wait for the next major version?

[isaacs]

@mistadikay can you clarify what you mean by "this error"? What are you trying to do, and how is it failing?

[mistadikay]

@isaacs oops, am I in a wrong thread? Sorry about that. I'm trying to use node-semver in webpack build and I have exactly the same error as in webpack/webpack#828 that dissapears when I remove import semver from 'semver' statement

[jasonkuhrt]

Great, the world become slightly simpler.

[dracupid]

It's a nice job to keep the module as clean as possible. But, when I install semver@5.0.0, AMD/Browser/minified bits are still there...Is there something wrong?

[isaacs]

Haha didn't make clean before this change. I'll release 5.0.1 tomorrow without those artifacts. Sorry about that.