Upgrading from 5.0.10 to 5.1.0 breaks existing tests: "TypeError: sinon_1.spy is not a function"

[fkleuver]

Describe the bug
Existing tests using spy fail with TypeError: sinon_1.spy is not a function.
This is since 5.1.0 (it quietly slipped in because I'm not using a lockfile at the moment.)

To Reproduce
Steps to reproduce the behavior:

1. git clone https://github.com/aurelia/experiment
2. cd experiment
3. yarn install
4. npm run test
5. See the error mentioned above
6. yarn add [email protected] -D
7. npm run test
8. No error

Expected behavior
Only 1 test should be failing instead of 25 :)

Context
Using a fairly large stack of libraries. I tried to create a repro with only the bare minimum, but then I got different errors (could not resolve chai), hence I'm referencing the full repo of where the error surfaced.

It should contain all the information you need, but I'd be happy to help with further debugging / fixing the issue if you wish. Just let me know.

Many thanks!

[fatso83]

Quite sure this is related to this #1805 (comment). I see that you are using TypeScript, in which case, it's the TypeScript bindings that are breaking with the new ES6 module support. We have no official support for those typings.

Closing on those grounds, but discussion is encouraged ..

[ujh]

I don't use TypeScript just "standard" ES6 via webpacker/babel and it's still broken: https://travis-ci.org/ujh/fountainpencompanion/jobs/389076772

[fatso83]

@ujh We ship a common js bundle module, as well as a browser bundle - and from the last minor release; a bundle exported as a ES2015 module. We haven't explicitly supported or tested for bundler compatibility up until that point. Simple upgrading from one Babel to another can make you have to replace lots of import foo from 'foo' with import * as foo from 'foo', so how bundlers deal with CommonJS exports has been a moving target. I see that it is an issue and an annoyance, but we can vouch for stuff we don't support.

Now that we actually do support using Sinon as a EcmaScript module, we will look into issues when it breaks - given the new module property in package.json.

[fkleuver]

@fatso83 I'm not entirely sure I understand the changes. If the current state is how you think it should be, that's completely fine. All else aside, this is a breaking change. That should at the very least be a major version bump, wouldn't you agree? Then it won't silently break existing apps - that's the point of semver.

I would say the best solution here is to revert the changes in a patch release 5.1.1 and then release these change in a major release 6.0.0. Then nothing will break and you can push forward with the new module system.

[fatso83]

@fkleuver There are two different aspects here: what is the correct and what would be the best thing for our users.

There is no breaking change with regards to what we support. We don't support Babel, Webpack, Typescript, Rollup, etc. It works, but how it works and how to fix that when it does, is outside of our scope. Up until now, we support using Sinon as a browser bundle and we support using it as a common js module. So if you managed to use it in other environments, great! But again, not something we support explicitly. If anything is a breaking change, it is that it either an API change, Sinon fails to load in a browser or Sinon fails to work in Node. Not working with bundlers: meh.

That covers the first aspect: onto the second one. Knowing that a lot of projects nowadays use transpilers and bundlers, and that this issue and other show that it breaks, I could be inclined to say that it is in the interest of our users to drop the semantics, New Jersey style, and do as you said: revert the commit for 5.x (seperate branch), publish the patch, ship a major version with the module to avoid our users needing to fix their code.

Input, @mroderick?

[fatso83]

I have prepared the necessary changes on a separate branch. I would just need to npm publish and merge the changelog into master for it to happen.

[fkleuver]

@fatso83 I appreciate you taking the time to explain and considering this. I got a little worried reading this comment

I understand the reasoning - you'd have to release every feature and bugfix as a breaking change if you wanted to be 100% sure to never break any potential thing.

However given the immense popularity of TypeScript and webpack, and the overall idea of trying your best as a library to not surprise users, it can't hurt to occasionally take the safe route and do a major bump for some large feature. Especially with a library that has so many dependents and is so intertwined with often complex testing stacks. Certainly romantic versioning is not an option then :)

It's completely fine and understandable if you don't anticipate this as a library developer. It would be unreasonable to expect that of you. Now that it's been pointed out, there's something you can do though.

I just so happened to figure this out because I was able to take the time to compare two lockfiles and go through the changes one by one until I found the cause. These issues are very difficult to track down when you have so many moving parts. It was sinon now, but it might have been a thousand other things.

Others might be losing days on it because they're got no clue what's going on. I promise you, that's happening.