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

3.0.1 is a breaking change #163

Closed
Daniel15 opened this issue Nov 30, 2016 · 12 comments
Closed

3.0.1 is a breaking change #163

Daniel15 opened this issue Nov 30, 2016 · 12 comments

Comments

@Daniel15
Copy link

Daniel15 commented Nov 30, 2016

See yarnpkg/yarn#2072

Version 3.0.1 should probably really be 3.1 as it changes the public API by renaming a file. This is not a patch release as per semver.

@broofa
Copy link
Member

broofa commented Nov 30, 2016

I'll let @defunctzombie weigh in on this, but IMHO (and the opinion of everyone in my office I just surveyed) files are not part of the programming interface. Static structure != dynamic behavior. "API" refers to the latter, not the former.

@navels
Copy link

navels commented Nov 30, 2016

certainly. but forgetting about the file structure, this worked in 3.0.0:

const uuid = require('uuid');

and now it does not.

@defunctzombie
Copy link
Collaborator

defunctzombie commented Nov 30, 2016 via email

@broofa
Copy link
Member

broofa commented Dec 1, 2016

Uh... that still works. Doesn't it?

kieffer@Roberts-MacBook-Pro$ npm install uuid
/Users/kieffer/work
`-- uuid@3.0.1

kieffer@Roberts-MacBook-Pro$ npm show uuid | grep version
  versions:
  version: '3.0.1',

kieffer@Roberts-MacBook-Pro$ node
> const uuid = require('uuid');
undefined
> uuid()
'53fcdf40-095f-4d82-b30c-a613884c69c4'

@broofa
Copy link
Member

broofa commented Dec 1, 2016

Also, node documents that {module}/index.js is part of the baseline module loading logic. See Step #2 under LOAD_AS_DIRECTORY, per https://nodejs.org/api/modules.html#modules_all_together

So... what exactly are we doing wrong here? I get that providing a main field in package.json might be a useful hint, but it's certainly not required... right?

@navels
Copy link

navels commented Dec 1, 2016

See yarnpkg/yarn#2072.

I'm not sure why your repro succeeds and our builds are failing. My workaround is to shrinkwrap our yarn installation so that we pull in uuid 3.0.0.

. . . time passes . . .

What's more, I can no longer reproduce our build failure using 3.0.1. Maybe we just blame it on cache.

@defunctzombie
Copy link
Collaborator

@broofa Agreed with your assessment. In my experience index.js is picked up as the canonical entry point for the module when no other main is specified. It is possible that someone had require('uuid/uuid.js'); and our changes would indeed break that. We can certainly move the index file back or symlink or just copy it and publish 3.0.2 to resolve these random issues.

On a related note, I personally don't have much sympathy for packages broken because they used ~ or ^ in their version specifiers for dependencies which I consider worse than an anti-pattern for node modules but that aside, we should still be good module citizens and make this simple fix on our end to ease the pain of those that have not yet learned the right lessons ;)

@Daniel15
Copy link
Author

Daniel15 commented Dec 1, 2016

Hmm interesting, I wonder if something else caused yarnpkg/yarn#2072 then. Do you have any ideas?

I personally don't have much sympathy for packages broken because they used ~ or ^ in their version specifiers for dependencies which I consider worse than an anti-pattern for node modules

~ is fine as it just allows upgrading the patch version. Patch releases are always supposed to be API/ABI compatible. Anything larger than small bug fixes should be a minor release (0.x.0). Moving stuff around does not constitute a bugfix 😛 . I agree that ^ is an anti-pattern though.

Having said that, this is one of the major reasons for using Yarn rather than npm - It enforces the usage of a lockfile, which locks in the version numbers of all packages including transitive dependencies. With npm you can use shrinkwrap but it's not as flexible.

@defunctzombie
Copy link
Collaborator

~ is fine as it just allows upgrading the patch version. Patch releases are always supposed to be API/ABI compatible.

Compatible with API/ABI doesn't actually mean bug compatible. Often engineering is done not just to the interfaces but to the actual observed behavior provided by those interfaces and when that changes even in the slightest (which can happen with dynamic languages with even just a change from == to === then the entire assumptions previously made may be invalidated.

@navels
Copy link

navels commented Dec 1, 2016

The failure in this case was in the request package used by yarn. Its package.json declares

"uuid": "^3.0.0"

and it fails with

no such file or directory, open '...some...path...here...\node_modules\uuid\uuid.js'

at this line in auth.js:

uuid = require('uuid')

I'm now back to being able to reproduce the error, but not by running node interactively. Will keep digging...

@navels
Copy link

navels commented Dec 2, 2016

Maybe we just blame it on cache.

Indeed, appears to have been a caching problem :)

@defunctzombie
Copy link
Collaborator

I am going to close this. Yes, 3.0.1 is by some definition a breaking change but not for anyone who used our documented require('uuid') which is all we have ever supported up to now. Yes you could do require('uuid/uuid.js') but no documentation said this nor did we intend to support it, it is just a by product of being able to require any file by referencing it relative to module root. In this same way many changes can be said to be breaking. Closing this issue until specific details are provided on a documented approach no longer working.

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

No branches or pull requests

4 participants