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

Added exports#default #107

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Added exports#default #107

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 3, 2018

Added exports#default, which adds support for things like "rollup-plugin-commonjs" which transpile commonJS modules into ES6 modules before transpilation, and other bundlers which do the same thing. It adds support for es6 modules, and it's only 7 letters, so it's worth it in the long run.

AAdded exports#default, which adds support for things like "rollup-plugin-commonjs" which transpile commonJS modules into ES6 modules before transpilation, and other bundlers which do the same thing. It adds support for es6 modules, and it's only 7 letters, so it's worth it in the long run.
Copy link
Author

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Sorry about the commit description typo.

@goto-bus-stop
Copy link
Member

doesn't rollup-plugin-commonjs not already convert module.exports = nanohtml to export default nanohtml?

@ghost
Copy link
Author

ghost commented Dec 3, 2018

@goto-bus-stop It doesn't bundle correctly. Not sharing source code, but the output is require calls in a umd bundle.

@goto-bus-stop
Copy link
Member

goto-bus-stop commented Dec 3, 2018

I get a correct bundle with the node-resolve and commonjs plugins: https://gist.github.com/goto-bus-stop/2189c8ea9759f52c3e07897f60f66d02

When I try it with this patch, rollup wraps nanomorph in a function (because (module.exports = expr).default is not easy to statically analyze), but it doesn't even use the .default value. it just uses the module.exports value directly anyway.

Bundle snippet
(function (global, factory) {
	typeof exports === 'object' && typeof module !== 'undefined' ? factory(require('assert')) :
	typeof define === 'function' && define.amd ? define(['assert'], factory) :
	(factory(global.assert));
}(this, (function (assert) { 'use strict';

	assert = assert && assert.hasOwnProperty('default') ? assert['default'] : assert;

	function createCommonjsModule(fn, module) {
		return module = { exports: {} }, fn(module, module.exports), module.exports;
	}

	var nanomorph_1 = createCommonjsModule(function (module) {
      // -- snip -- 

	(module.exports = nanomorph).default = nanomorph;

      // -- snip --
	});

    // vv no .default here vv
	nanomorph_1(document.body, browser`
  <body>
    <div id="test">
      <h1>Hello world</h1>
    </div>
  </body>
`);

})));

What's different about our configs/source code?

@ghost
Copy link
Author

ghost commented Dec 4, 2018

@goto-bus-stop
Copy link
Member

is that error fixed by this patch? it looks totally unrelated… i have no idea where it's getting that src/util.ts from, the rollup-plugin-node-builtins package doesn't contain any typescript.

@ghost
Copy link
Author

ghost commented Dec 7, 2018

@goto-bus-stop I don't know why, but for some reason Rollup is hooking the node util package to my /src/util.ts package. Looks like a bug with rollup-plugin-node-builtins.

@goto-bus-stop
Copy link
Member

i don't know either.

as for this patch, i'm not opposed to it in principle, but i still don't understand how it fixes anything. the errors you're sharing appear entirely unrelated.

for it to be accepted, the style changes would have to be reverted so that it passes standard, and imo the .default assignment should be on its own line to be more readable and to allow static analysis tools to understand the code.

@ghost
Copy link
Author

ghost commented Dec 7, 2018

Yeah, sorry about the offtopic stuff. Was doing two different things at once. Anyway, I've seen other libraries do this exports.default thing, so I know it's important and it seems to fix the issue for me.

EDIT: Would also like to add that this fixes nanomorph for people that don't use the interop block in Rollup.

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

1 participant