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

Fixup system - follow up on #4356 #4373

Merged
merged 4 commits into from Mar 1, 2017
Merged

Fixup system - follow up on #4356 #4373

merged 4 commits into from Mar 1, 2017

Conversation

timse
Copy link
Member

@timse timse commented Feb 24, 2017

followup on #4356

see there for detailed description

Copy link
Member

@sokra sokra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not good that you mixed ES6 refactoring with other changes in this PR! This makes reviewing difficult. Stay focused with PRs.

@@ -1,5 +1,5 @@
it("should polyfill System", function() {
if (typeof System === "object" && typeof System.register === "function") {
if (typeof System.register === "function") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are changes to the tests necessary?

parser.state.current.addVariable(name, expression, deps);
return true;
};
module.exports = class ParserHelpers {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ParserHelpers is not a real class. Stay with the exports.addParsedVaraibleToModule style. It's more aligned with the future style export function addParsedVariableToModule().

@@ -45,7 +45,6 @@ it("should not parse filtered stuff", function() {
if(typeof module === "undefined") module = require("fail");
if(typeof module != "object") module = require("fail");
if(typeof exports == "undefined") exports = require("fail");
if(typeof System !== "object") exports = require("fail");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are changes to the tests necessary?

typeof System should object in webpack.

@timse
Copy link
Member Author

timse commented Feb 28, 2017

reverted the refactor.
need to check again why i needed to change the typeof checks i think i had a reason :)

@timse
Copy link
Member Author

timse commented Feb 28, 2017

changes are reverted.
thanks @sokra for taking time with this 👍

@@ -1,3 +1,6 @@
/* global System */
// "fix" for users of "System" global
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix comment

use webpacks "own" System polyfill, do not try to access a previously existing one.
Instead make sure everything will be "shadowed" by the webpack System. thx @sokra for the help!
@sokra sokra merged commit 6ef2c75 into webpack:master Mar 1, 2017
@sokra
Copy link
Member

sokra commented Mar 1, 2017

Thanks

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

Successfully merging this pull request may close these issues.

None yet

3 participants