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

1.24.0 is not semver-minor #19

Open
timdp opened this issue Sep 11, 2017 · 5 comments
Open

1.24.0 is not semver-minor #19

timdp opened this issue Sep 11, 2017 · 5 comments

Comments

@timdp
Copy link

timdp commented Sep 11, 2017

In d2bd46b, you upgraded Rollup from 0.45.1 to 0.49.2. However, Rollup's config API recently made a few breaking changes, meaning that rollup-stream version 1.24.0 isn't backwards compatible with 1.23.0, which violates semver.

Rollup-stream copes with entry being renamed to input, but there are other deprecations. In my case, it would crash on useStrict, which is now called strict.

To align with Rollup's 0.x versioning, theoretically, every minor upgrade to Rollup would have to translate to a major upgrade to rollup-stream. I suppose you could also introduce lots of code to deal with Rollup's deprecated APIs, but that seems like something for a different package, at most.

Anyway, I mainly wanted to flag this so at least everyone's aware of it. Thanks.

@lemmabit
Copy link
Owner

Yeah, I tried to paper over this by renaming sourceMap to sourcemap (Rollup handles entryinput on its own). I knew there were other removals but didn't think about them for some reason.

I can't remember why I published rollup-stream@1.0.0 instead of 0.0.0 two years ago. It might have been due to gulp-rollup's influence, a package I now maintain but didn't originally create and which has the same problem.

What I'd like to do is re-publish as v0.0.0 and put an npm deprecate on the 1.x.x series. But I don't know what the ramifications of that would be for things like david-dm.org.

If I really wanted to, I could just remove the "for-convenience" dependency on Rollup and require users to pass in options.rollup, making Rollup's public API no longer a part of rollup-stream's. Then I'd only need one major version bump, ever, for that one change.

But this package has gone this long without having to get a major version bump due to changes to Rollup. Maybe it can afford it this one time as long as Rollup doesn't make a habit of it.

It's a bit infuriating to me that they decided to change the API to match the command line options rather than the other way around. No one would have had to update every instance of entry in their tests or deal with semver problems if they'd done it the other way. But they seem to look down their noses at API consumers, like the command line version is all any reasonable person would ever need and we barbarians can fend for ourselves. What a pain.

@Andarist
Copy link

If I really wanted to, I could just remove the "for-convenience" dependency on Rollup and require users to pass in options.rollup, making Rollup's public API no longer a part of rollup-stream's. Then I'd only need one major version bump, ever, for that one change.

This would for sure be a better option out of those 2, aint sure if its that easy though. Im not that familiar with rollup's implementation but I believe it has changed some of its APIs recently (few versions back) from synchronous to Promise-based. Also you are trying to follow what rollup's CLI is doing (from what I understand) and do something like this:

rollup.rollup(options).then(function(bundle) {
  return bundle.generate(outputOptions);
}).then(function(result) {
  // ...
})

Those lines are probably also rollup version specific and you'd have to abstract it away somehow. Probably something could be done on rollup's side to ease integrations.

dfreedm added a commit to webcomponents/shadycss that referenced this issue Sep 19, 2017
Lock rollup-stream to 1.23 for lemmabit/rollup-stream#19
Update closure compiler to 20170910
@lemmabit
Copy link
Owner

@Andarist The compatibility concerns you've raised have never been problems for me in practice. The transition to the Promise-based generate required very little effort and was delightfully transparent, and the lines you cited will work exactly the same in every Rollup version I've ever dealt with as far as I know.

How's this for rollup-stream v2.0.0:

var rollup = require('rollup-stream')(require('rollup'));

After that, it works exactly like it always has. Even if Rollup has breaking changes that require adjustments to rollup-stream, supporting those new versions is strictly semver-minor as long as the package still functions with the old versions. I may have to check rollup.VERSION to know how it expects to be used.

@Andarist
Copy link

@Permutatrix this seems to be cool :) I thought it would require more effort

@timdp
Copy link
Author

timdp commented Sep 20, 2017

For bonus points, you could inspect the call stack to see which package is requiring yours and use the resolve package to find rollup relative to that one. Or you could support passing a string-typed argument which is the __dirname from which to resolve rollup. Just some ideas to avoid typing require twice. 🙂

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

3 participants