-
Notifications
You must be signed in to change notification settings - Fork 525
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
Replace default exports with individual exports and namespaced imports #1907
Conversation
import * as Helpers from "../victory-util/helpers"; | ||
import * as LabelHelpers from "../victory-util/label-helpers"; | ||
import * as Style from "../victory-util/style"; | ||
import * as Log from "../victory-util/log"; | ||
import TextSize from "../victory-util/textsize"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still working on TextSize - this one broke some tests and I'm still determining whether this is an issue with sinon mocking or some other kind of bug. Storybook looks the same, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@beccanelson If you're still spinning your wheels on this, feel free to just give me some reproduction instructions and I can take a look too!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured it out - the earlier version of sinon we were using for test stubbing doesn't support module exports. Upgrading sinon seems to be doing the trick to fix the tests.
@beccanelson This is all looking great to me! Please remove the draft status when you're ready so the chromatic tests aren't skipped |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I just pushed up the sinon update, which should fix some of the test mocking issues I was having. Do we also want to include all the |
Hmm, maybe there's something else going on here. I still see some of the test failures related to the TextSize module. |
@beccanelson the tests I'm seeing fail look like they're related to |
@boygirl Yes, from what I can figure out we're trying to stub the result of |
ah, gotcha |
const stub = sandbox | ||
.stub(pathHelpers, symbol) | ||
.returns(`${symbol} symbol`); | ||
stub = sinon.stub(pathHelpers, symbol).returns(`${symbol} symbol`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is an error in the test before this line of code your stub.restore()
call in line 23 with also fail. Better to switch back to the sandbox approach because that is guaranteed to be instantiated in beforeEach
@@ -240,6 +240,27 @@ const _approximateTextHeightInternal = (text, style) => { | |||
}, 0); | |||
}; | |||
|
|||
// Stubbable implementation. | |||
export const _approximateTextSizeInternal = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@boygirl Do you have thoughts about this stubbable implementation that @ryan-roemer added? Normally I'm not a huge fan of doing hacky stuff like this for the sake of testing, but I also don't want to lose the benefit of the existing tests or spend a ton of time trying to figure out why this particular module isn't stubbable.
The other option we have (which probably isn't much better) is to do some old-fashioned dependency injection and accept the text size as an optional prop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@beccanelson I agree that the stubbable implementation isn't great, but I think it's the best option for the moment. Let's go with that so you don't have to keep spinning your wheels on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree with y’all. 😛 If it was more than just one, we’d probably want to look for more first class solutions.
449e0e6
to
27c9c77
Compare
I rebased on main, and got some test errors that can be traced back to f873926. It seems like the latest sinon version and its dependencies are not playing well with webpack 5. 😭 |
@beccanelson I don't have time for this today, but I can dig into this tomorrow if you'd like? |
@ryan-roemer No rush on this, but I would appreciate some help whenever you're free! |
I'm up to date on this branch and getting the same error as CI, namely:
I think this may indicate potential issues with our upstream consumers for the exports change, so I'll dig in more tomorrow about implications. ( |
fe42e79
to
3742284
Compare
Here's what is currently failing: /*!*************************************************************!*\
!*** ../../node_modules/parse5/lib/parser/parser_stream.js ***!
\*************************************************************/
/***/ ((module, __unused_webpack_exports, __webpack_require__) => {
"use strict";
var WritableStream = __webpack_require__(/*! stream */ "?2cec").Writable,
inherits = __webpack_require__(/*! util */ "../../node_modules/util/util.js").inherits,
Parser = __webpack_require__(/*! ./index */ "../../node_modules/parse5/lib/parser/index.js");
var ParserStream = module.exports = function (options) {
WritableStream.call(this);
this.parser = new Parser(options);
this.lastChunkWritten = false;
this.writeCallback = null;
this.pausedByScript = false;
this.document = this.parser.treeAdapter.createDocument();
this.pendingHtmlInsertions = [];
this._resume = this._resume.bind(this);
this._documentWrite = this._documentWrite.bind(this);
this._scriptHandler = this._scriptHandler.bind(this);
this.parser._bootstrap(this.document, null);
};
// ==> THIS IS FAILING <===
inherits(ParserStream, WritableStream);
//WritableStream implementation
ParserStream.prototype._write = function (chunk, encoding, callback) {
this.writeCallback = callback;
this.parser.tokenizer.write(chunk.toString('utf8'), this.lastChunkWritten);
this._runParsingLoop();
};
ParserStream.prototype.end = function (chunk, encoding, callback) {
this.lastChunkWritten = true;
WritableStream.prototype.end.call(this, chunk || '', encoding, callback);
};
//Scriptable parser implementation
ParserStream.prototype._runParsingLoop = function () {
this.parser.runParsingLoopForCurrentChunk(this.writeCallback, this._scriptHandler);
}; |
This must have been an existing bug - there was a misnamed function here that was throwing an error with the new export syntax
@ryan-roemer @boygirl I added a node polyfill to the test config that seems to fix the issue in |
Closes #650
Closes #1909
Updates default export objects to individual exports to better support webpack tree-shaking. In order to preserve the namespacing (e.g.
import { Helpers } from 'victory-core'
), this adds the babel export namespace proposal in order to support syntax likeexport * as Helpers from 'victory-util/helpers'
, though it sounds like we might need to upgrade to webpack v5 to support tree-shaking with this syntax.I am currently running into this issue with any helper methods that are mocked using
sinon.stub(...)
in the tests. I will continue to investigate this!This also includes some testing improvements, including a
TEST_MODULE
env variable to isolate a single module. For example, now you can run:This also adds test watcher scripts so you can run
nps test.watch
ornps karma.watch
.