Throw meaningful error stubbing ECMAScript Module #1715
Conversation
lib/sinon/stub.js
Outdated
| typeof Symbol !== "undefined" && | ||
| object[Symbol.toStringTag] === "Module" | ||
| ) { | ||
| throw new TypeError("No support for stubbing ES Modules"); |
There was a problem hiding this comment.
This sound a bit like "Sinon does not have this feature", but really, it's impossible. Maybe "ES modules cannot be stubbed"?
| const {assert} = referee; | ||
|
|
||
| describe("Explicit lack of support for stubbing Modules", function() { | ||
| it("should give a proper error message for modules without default export", function() { |
There was a problem hiding this comment.
Any export (or rather import) would be read-only, wouldn't it? This seems to suggest that read-only only applies to default exports.
There was a problem hiding this comment.
I was thinking of adding some additional tests, but couldn't be bothered at that time of the night :-) I added some more tests now, and as you can see, it's only-halfway true. You can actually stub some forms of exports. Although you can't modify the exported properties, one can modify the objects that has been exported (check the content of the tests), so one can modify the props of export default { a: 1, b: 2 }, just not point default to another object.
| describe("Explicit lack of support for stubbing Modules", function() { | ||
| it("should give a proper error message for modules without default export", function() { | ||
| assert.exception(function() { | ||
| sinon.stub(aModule, "anExport").returns(400); |
There was a problem hiding this comment.
We will have the same problem for spies, won't we?
There was a problem hiding this comment.
Yes. Pushed a fix for this.
lib/sinon/stub.js
Outdated
| @@ -18,6 +18,14 @@ function stub(object, property) { | |||
| throw new TypeError("stub(obj, 'meth', fn) has been removed, see documentation"); | |||
| } | |||
|
|
|||
| if ( | |||
There was a problem hiding this comment.
It would be great to move this into a function, so it can be re-used (upcoming sinon@next has a new replace method).
There was a problem hiding this comment.
All comments addressed and more tests added!
sinon.stub()
Modules with objects as their default export
✓ should NOT result in error
✓ should spy/stub an exported function
Modules without default export
✓ should give a proper error message
Modules that exports a function as their default export
✓ should not be possible to spy/stub the default export using a wrapper for the exports
sinon.spy()
Modules with objects as their default export
✓ should NOT result in error
✓ should spy/stub an exported function
Modules without default export
✓ should give a proper error message
Modules that exports a function as their default export
✓ should not be possible to spy/stub the default export using a wrapper for the exports
8 passing (17ms)
| describe("Explicit lack of support for stubbing Modules", function() { | ||
| it("should give a proper error message for modules without default export", function() { | ||
| assert.exception(function() { | ||
| sinon.stub(aModule, "anExport").returns(400); |
There was a problem hiding this comment.
Yes. Pushed a fix for this.
| const {assert} = referee; | ||
|
|
||
| describe("Explicit lack of support for stubbing Modules", function() { | ||
| it("should give a proper error message for modules without default export", function() { |
There was a problem hiding this comment.
I was thinking of adding some additional tests, but couldn't be bothered at that time of the night :-) I added some more tests now, and as you can see, it's only-halfway true. You can actually stub some forms of exports. Although you can't modify the exported properties, one can modify the objects that has been exported (check the content of the tests), so one can modify the props of export default { a: 1, b: 2 }, just not point default to another object.
| var stub; | ||
| var errorRegEx = /TypeError: ES Modules cannot be (stubbed|spied)/; | ||
|
|
||
| afterEach(function() { |
There was a problem hiding this comment.
This seems duplicated with the same afterEach inside the describe below ... is that intentional?
|
|
||
| describe("sinon." + action + "()", function(){ | ||
| describe("Modules with objects as their default export", function() { | ||
| afterEach(function() { |
There was a problem hiding this comment.
Hopefully ESLint complains about indentation here ...
There was a problem hiding this comment.
No it did not, but I have now added a lint-staged script to catch this.
Ref #1711 and #1623 for background. This feature is simply about throwing a meaningful error, instead one that tells the user that the property cannot be deleted. As the exports from ECMAScript modules are immutable once created we cannot do anything about them from the view of Sinon, so it's preferable to simply tell the user that when we can. This should not affect transpiled code (say, using Babel), as the resulting modules are no longer true ECMAScript modules (but rather some form of CommonJS module).
Also fixes the linting issues and some feedback from the PR.
fatso83
force-pushed
the
sinon-es6-module-detection
branch
from
0d35d47
to
51cdafe
Compare
Purpose (TL;DR)
Throw a meaningful error when encountering ES Modules in
sinon.stub()Background
Ref #1711 and #1623 for background.
This feature is simply about throwing a meaningful error, instead of one that tells the user that the property cannot be deleted. As the exports from ECMAScript modules are immutable once created we
cannot do anything about them from the view of Sinon, so it's preferable to simply tell the user that when we can.
This should not affect transpiled code (say, using Babel), as the resulting modules are no longer true ECMAScript modules (but rather some form of CommonJS module).
How to verify - mandatory
npm installnpm run test-es-module(included innpm test)Checklist for author
npm run lintpasses