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

Bugfix/disable module in harmony #3755

Merged
merged 5 commits into from
Jan 9, 2017
Merged

Conversation

sokra
Copy link
Member

@sokra sokra commented Jan 4, 2017

What kind of change does this PR introduce?
bugfix

Did you add tests for your changes?
yes

If relevant, link to documentation update:
N/A

Summary
emit errors if using AMD or CommonJS syntax in harmony modules
and some refactorings needed for this

Does this PR introduce a breaking change?
yes

@sokra sokra force-pushed the bugfix/disable-module-in-harmony branch 2 times, most recently from ffbef21 to f56e5d5 Compare January 4, 2017 18:25
@sokra sokra force-pushed the bugfix/disable-module-in-harmony branch 5 times, most recently from 04938c8 to e8d1271 Compare January 6, 2017 02:57
@sokra sokra force-pushed the bugfix/disable-module-in-harmony branch from e8d1271 to 828d5e3 Compare January 8, 2017 20:45
@TheLarkInn
Copy link
Member

Looks good. Still working on this?

this.message += err.message;
this.origin = this.module = module;
this.error = err;
}
Copy link
Member

Choose a reason for hiding this comment

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

class ModuleDependencyError extends Error {
  constructor(module, err, loc) {
    super(`${formatLocation(loc)} ${err.message}`);
    this.name = "ModuleDependencyError";
    this.details = err.stack.split("\n").slice(1).join("\n"); // err.stack.replace(/^./g, "")?
    this.origin = this.module = module;
    this.error = err;
  }
}

Calling Error.captureStackTrace is not necessary on native subclasses. The message can also just be given as the super argument instead of assigning to this.message. However, not calling Error.captureStackTrace does leave the constructor itself as the top of the stack trace.

@@ -2,12 +2,14 @@
MIT License http://www.opensource.org/licenses/mit-license.php
Author Tobias Koppers @sokra
*/
var formatLocation = require("./formatLocation");
Copy link
Member

Choose a reason for hiding this comment

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

const

var dep = new CommonJsInHarmonyDependency(this.state.module, "exports");
dep.loc = expr.loc;
this.state.current.addDependency(dep);
});
Copy link
Member

@Jessidhia Jessidhia Jan 9, 2017

Choose a reason for hiding this comment

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

All of these should only match if the user doesn't shadow them with their own binding, otherwise this would prevent using e.g. module as an identifier (as often happens in webpack's own code).

Copy link
Member Author

Choose a reason for hiding this comment

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

The Parser should automatically handle this. But I'll add a test case validating it.

var dep = new CommonJsInHarmonyDependency(this.state.module, "module");
dep.loc = expr.loc;
this.state.current.addDependency(dep);
});
Copy link
Member

Choose a reason for hiding this comment

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

Any escape hatch (or alternative) for module.hot? Or it is not a problem as a more specific expression module.hot plugin would run before?

Copy link
Member Author

Choose a reason for hiding this comment

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

expression module.hot will run before expression module. As expression module.hot returns false it stops traversion.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is already tested by the HMR testcases for harmony modules.

Error.captureStackTrace(this, CommonJsInHarmonyWarning);
this.name = "CommonJsInHarmonyWarning";
this.message = name + " is not allowed in EcmaScript module: This module was detected as EcmaScript module (import or export syntax was used). In such a module using '" + name + "' is not allowed.";
}
Copy link
Member

Choose a reason for hiding this comment

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

class CommonJsInHarmonyWarning extends Error {
  constructor (name) {
    super(`${name} is not allowed in EcmaScript module: This module was detected as EcmaScript module (import or export syntax was used). In such a module using '${name}' is not allowed.`)
    this.name = "CommonJsInHarmonyWarning"
  }
}

xc as xc,
xd as xd
}
Copy link
Member

Choose a reason for hiding this comment

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

export { xa, xb, xc as xc, xd as xd } from "./module"?

or is this specifically checking export of imported symbol?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is an old test and I didn't want to change it more than necessary.

@TheLarkInn TheLarkInn merged commit 9380bd4 into master Jan 9, 2017
@TheLarkInn
Copy link
Member

Looks good.

@TheLarkInn
Copy link
Member

TheLarkInn commented Jan 9, 2017

The Parser should automatically handle this. But I'll add a test case validating it.

Sorry I missed this. @sokra do you want a separate issue for that?

@sokra
Copy link
Member Author

sokra commented Jan 9, 2017

uh you already merged it...

I'll create a new PR...

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