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

syntheticNamedExports #60

Closed
CodeWitchBella opened this issue Jan 5, 2020 · 6 comments
Closed

syntheticNamedExports #60

CodeWitchBella opened this issue Jan 5, 2020 · 6 comments

Comments

@CodeWitchBella
Copy link
Contributor

Rollup just released new option for plugins named syntheticDefaultExports. It allows for better implementation of commonjs plugin without need to manually specify named exports which I found to be the biggest obstacle to using rollup.

Link to rollup PR: rollup/rollup#3295

Creating this issue only to bring this option to your attention. Thanks for awesome project.

@PepsRyuu
Copy link
Owner

PepsRyuu commented Jan 5, 2020

Thanks for the information, wasn't aware at all! Seems like a very powerful feature.

I think it should be very easy to replicate the feature for synchronous use cases (ie. CommonJS). When generating the file output, if syntheticNamedExports is passed, then I can just do the following:

__export('default', SomeDefaultExport);

// synthetic named exports
for (var prop in SomeDefaultExport) { 
    __export(prop, SomeDefaultExport[prop]);
}

Asynchronous use cases (ie. instantiating WASM using streaming) would be a bit more complicated as it requires a live binding implementation, but it's not a design goal for Nollup at the moment due to the processing complexity of it.

For the time being I think this will suffice, and will allow people to seamlessly use CommonJS. I'll update the example as well to remove usage of "namedExports".

If there's any concerns with this approach let me know! 🙂

@PepsRyuu
Copy link
Owner

PepsRyuu commented Jan 5, 2020

0da4538

Initial implementation. Will be tidying it up with test cases and releasing soon. Works well with the example projects.

@CodeWitchBella
Copy link
Contributor Author

Wow! That was fast 🙂

I think that the best would be to stop detecting exports in rollup-plugin-commonjs-alternate and maybe just append something like var module = { exports: {} }; var exports = module.exports; export default module.exports;

This would fix following code (taken from older version of validator, nowadays they have es modules):

"use strict";

Object.defineProperty(exports, "__esModule", {
  value: true
});
exports.default = isEmail;


function isEmail(str, options) {
  return 'hello world'
}

module.exports = exports.default;
module.exports.default = exports.default;

This is currently broken in commonjs alternaternate because it keeps last two lines intact.

But we can also wait what @rollup/plugin-commonjs comes up with - it would be great if it started working with nollup so that users don't have to change their configs

@PepsRyuu
Copy link
Owner

PepsRyuu commented Jan 5, 2020

Should be fixed in 0.1.1 of the plugin.

The reason for the new plugin is mostly due to difference of opinion in how different situations should be approached. For example, the official plugin does not support dynamic requires, which is necessary for HMR to work. Since Nollup isn't an official feature of Rollup or anything like that, it's not considered for the official plugin to support so they stub out dynamic requires and throw out an error. From their perspective, it's more user-friendly. Another reason is the ES module detection. React Hot Loader adds CJS requires to ES modules, which the official plugin doesn't support without significant changes and possible performance loss, and doesn't make sense for them to add if HMR isn't an official feature.

module.exports can't be mocked, because it would shadow the module object coming in from Nollup, which provides module.id and module.hot. However I took some inspiration from your suggestion and decided to alias var exports = __exports and it works perfectly now! Thanks for the tip! :)

PepsRyuu/rollup-plugin-commonjs-alternate@2322a5e

@CodeWitchBella
Copy link
Contributor Author

CodeWitchBella commented Jan 6, 2020

That makes sense. This is great improvement because biggest pain with using (r|n)ollup was having to configure namedExports manually. Thank you 🙂

@PepsRyuu
Copy link
Owner

PepsRyuu commented Jan 6, 2020

Released in 0.10.0 :)

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

No branches or pull requests

2 participants