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

Deal with bundler issues #1829

Merged
merged 6 commits into from Jun 11, 2018

Conversation

Projects
None yet
4 participants
@fatso83
Contributor

fatso83 commented Jun 7, 2018

Ref the discussion regarding Webpack and other ES2015+ bundlers in #1805 and #1828 releasing a new major versionseems like a pragmatic, low-effort way of keeping noise out of our issue tracker.

This goes along with releasing a patch for 5.x.

See #1805 and #1828 for details. The 'module' field in package.json caught some people by surprise.

@fatso83 fatso83 requested a review from mroderick Jun 7, 2018

@coveralls

This comment has been minimized.

coveralls commented Jun 7, 2018

Coverage Status

Coverage remained the same at 85.299% when pulling 7e79b26 on fatso83:version-6-with-esm-export-for-bundlers into 57f4077 on sinonjs:master.

@mroderick

This comment has been minimized.

Member

mroderick commented Jun 8, 2018

We usually provide a migration guide for each major version. Could we add one for this release as well?

@fatso83

This comment has been minimized.

Contributor

fatso83 commented Jun 8, 2018

Sure ... but I am not sure what should be in it, since I am not consuming Sinon as a bundler target in any of my own projects ...

@mroderick

This comment has been minimized.

Member

mroderick commented Jun 8, 2018

@fatso83 will you make the 5.1.1 pull request, so we can merge it and fix people's projects?

@fatso83

This comment has been minimized.

Contributor

fatso83 commented Jun 8, 2018

sure

@mroderick

This comment has been minimized.

Member

mroderick commented Jun 8, 2018

I'm impatient 😉

#1834

@fatso83

This comment has been minimized.

Contributor

fatso83 commented Jun 8, 2018

I am not quite sure what to put in the 6.0 migration ... I am wondering if we should just put a placeholder there and release it now, updating it in hindsight, or if we should gather some experience with what to do with troubles affected by this?

I don't have access to any such projects of my own, atm. Remember if we got a link to such a thing recently?

@mroderick

This comment has been minimized.

Member

mroderick commented Jun 8, 2018

I am not quite sure what to put in the 6.0 migration ... I am wondering if we should just put a placeholder there and release it now, updating it in hindsight, or if we should gather some experience with what to do with troubles affected by this?

That's a good question.

We let the pre-release of 5 be available for a long time, and it seems that @mantoni was the only one that really took it for a spin. I don't expect a pre-release of 6 to be much different.

I don't have access to any such projects of my own, atm. Remember if we got a link to such a thing recently?

Are you saying that you don't have access to any projects that use all the bundling toys?

@fatso83

This comment has been minimized.

Contributor

fatso83 commented Jun 11, 2018

Are you saying that you don't have access to any projects that use all the bundling toys?

For some weird reason 😄 But I got a tip in #1835 to test out with chrisguttandin/midi-player and it seems there is no reason to have a migration guide for 6.0, as there doesn't seem to be any immediate issues now that #1833 has been implemented.

Bundler issues starting appearing with the module field in Sinon 5.1.0:

npm install && npm install sinon@5.1.0 && npm test

WARNING in ./test/mock/performance.js 4:9-13
"export 'stub' was not found in 'sinon'
 @ ./test/unit/midi-player.js

HeadlessChrome 0.0.0 (Linux 0.0.0) ERROR
...
HeadlessChrome 0.0.0 (Linux 0.0.0): Executed 0 of 6 SUCCESS (0 secs / 0 secs)
Firefox 60.0.0 (Ubuntu 0.0.0) ERROR

After running with the local Sinon 6 version, using npm link sinon:

HeadlessChrome 0.0.0 (Linux 0.0.0): Executed 9 of 9 SUCCESS (0.064 secs / 0.029 secs)
HeadlessChrome 0.0.0 (Linux 0.0.0): Executed 9 of 9 SUCCESS (0.064 secs / 0.029 secs)
Firefox 60.0.0 (Ubuntu 0.0.0): Executed 9 of 9 SUCCESS (0.09 secs / 0.014 secs)
TOTAL: 18 SUCCESS

@fatso83 fatso83 merged commit 05d7990 into sinonjs:master Jun 11, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@fatso83 fatso83 deleted the fatso83:version-6-with-esm-export-for-bundlers branch Jun 11, 2018

@fatso83

This comment has been minimized.

Contributor

fatso83 commented Jun 11, 2018

Sinon 6 is out (with migration docs).

@mroderick

This comment has been minimized.

Member

mroderick commented Jun 11, 2018

Excellent!

@janis91

This comment has been minimized.

janis91 commented Jun 12, 2018

@fatso83 When you click "Releases" on the website and then "Docs" for v6.0.0 the migration guide is not linked. Maybe this is something that could be changed, too. I just wondered why there is no migration guide for v6.0.0. I found this issue linked in one of the last commits in the repo, but I was not looking in the "Guides" section. Maybe this is something that other users may stumble upon as well.

@fatso83

This comment has been minimized.

Contributor

fatso83 commented Jun 12, 2018

This is me not knowing the structure of the site well enough. I remembed docs/guides/index.md b/docs/guides/index.md, but failed to see there is a "duplicate" file in https://github.com/sinonjs/sinon/blob/master/docs/_includes/docs/migration-guides.md. Seems this should be included in the other to avoid this situation appearing.

@fatso83

This comment has been minimized.

Contributor

fatso83 commented Jun 12, 2018

The "guide" is linked from the Guides section, btw.

fatso83 added a commit to fatso83/sinon that referenced this pull request Jun 14, 2018

Use migration guide include in guide index
Avoid landing in the situation where we remember to update
one place, but not the other.

Ref sinonjs#1829 (comment)
@fatso83

This comment has been minimized.

Contributor

fatso83 commented Jun 14, 2018

@janis91 The doc issue will be fixed by #1838. Thanks for noticing it!

@janis91

This comment has been minimized.

janis91 commented Jun 14, 2018

You're welcome :-)

fatso83 added a commit that referenced this pull request Jun 14, 2018

Use migration guide include in guide index (#1838)
Avoid landing in the situation where we remember to update one place, but not the other.

Ref #1829 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment