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

Use module.watch(require(id), ...) to avoid calling module.require. #159

Merged
merged 1 commit into from
May 9, 2017

Conversation

benjamn
Copy link
Owner

@benjamn benjamn commented May 8, 2017

If we choose to go in this direction, we would be removing a significant barrier to interop with CommonJS module systems other than Node (in particular, Webpack and Browserify), since module.require would no longer be a requirement, and third-party bundling tools could easily understand dependencies embedded in module.watch(require(id), ...) calls.

I'm not committed to module.watch as a method name, though it's short and accurately reflects that the parent module is watching an exports object returned by a child module.

@benjamn benjamn requested a review from jdalton May 8, 2017 23:27
@benjamn
Copy link
Owner Author

benjamn commented May 8, 2017

A really interesting question is whether a package that uses a version of Reify that generates module.importSync calls could work alongside a package that generates module.watch calls. I think probably yes, but it would be nice to verify.

lib/runtime.js Outdated
if (utils.isObject(setters)) {
Entry.getOrCreate(exported).addSetters(this, setters, key);
}
return moduleWatch.call(this, this.require(id), setters, key);
Copy link
Contributor

Choose a reason for hiding this comment

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

this.watch(this.require(id), setters, key)

@jdalton
Copy link
Contributor

jdalton commented May 8, 2017

Would this deprecate importSync, with the plan of removing it in a near future version?

@jdalton
Copy link
Contributor

jdalton commented May 9, 2017

Heads up: I rebased this PR.

After this is merged it may be a good time to revisit the problem of referencing module and require in the generated code (since user code could redefine them). I'm cool just creating unique-ish vars and using them. By creating unique-ish vars I'm mean something as simple as __reifyModule and __reifyRequire 😋

@benjamn
Copy link
Owner Author

benjamn commented May 9, 2017

Would this deprecate importSync, with the plan of removing it in a near future version?

Yes, I think importSync will be unnecessary in the future, and we should remove it, though I vaguely worry about breaking existing generated code by changing the runtime.

@benjamn benjamn force-pushed the module.watch branch 2 times, most recently from de18e05 to ce6bd37 Compare May 9, 2017 00:35
@jdalton
Copy link
Contributor

jdalton commented May 9, 2017

Cool. We could deprecate it in v0.11 and remove it in v0.12 (that's being super nice)

@benjamn
Copy link
Owner Author

benjamn commented May 9, 2017

Do you have any preferred techniques for giving not-too-noisy deprecation warnings?

@jdalton
Copy link
Contributor

jdalton commented May 9, 2017

I'd put it in the readme for v0.11 and you can add a note on npm install with npm deprecate just for v0.11.

pad(this, "module.importSync(", decl.start, decl.source.start) +
getSourceString(this, decl) +
pad(this, "module.watch(", decl.start, decl.source.start) +
"require(" + getSourceString(this, decl) + ")" +
Copy link
Contributor

@jdalton jdalton May 9, 2017

Choose a reason for hiding this comment

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

Should the "require(" + getSourceString(this, decl) + ")" bit be in either the first pad() or the following one?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good catch, I think it should go in the one above, since it's usually (always?) only one line, and that line is the first line of the module.watch expression.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Fixed, thanks!

@benjamn
Copy link
Owner Author

benjamn commented May 9, 2017

After this is merged it may be a good time to revisit the problem of referencing module and require in the generated code (since user code could redefine them). I'm cool just creating unique-ish vars and using them. By creating unique-ish vars I'm mean something as simple as __reifyModule and __reifyRequire 😋

I definitely agree we should do that for module. I think require is much safer, though, since most people know they'll mess up their Webpack bundles if they rename/shadow the require function. Also, we want bundlers to understand our usage of the require function, and module.watch(__reifyRequire("source"), ...) would be just as confusing as module.importSync("source", ...).

If we choose to go in this direction, we would be removing a significant
barrier to interop with CommonJS module systems other than Node (in
particular, Webpack and Browserify), since module.require would no longer
be a requirement, and third-party bundling tools could easily understand
dependencies embedded in module.watch(require(id), ...) calls.

I'm not committed to module.watch as a method name, though it's short and
accurately reflects that the parent module is watching an exports object
returned by a child module.
@jdalton jdalton merged commit a61d310 into master May 9, 2017
@jdalton jdalton deleted the module.watch branch May 9, 2017 15:08
@benjamn
Copy link
Owner Author

benjamn commented May 9, 2017

  • Note to self: update the README.md to reflect these changes. 2c78577

@benjamn benjamn added this to the Release 0.11.0 milestone May 9, 2017
benjamn pushed a commit to meteor/meteor that referenced this pull request May 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants