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

riot@3.0.0 should export default #2084

Closed
Joylei opened this issue Nov 23, 2016 · 9 comments
Closed

riot@3.0.0 should export default #2084

Joylei opened this issue Nov 23, 2016 · 9 comments
Labels

Comments

@Joylei
Copy link
Contributor

@Joylei Joylei commented Nov 23, 2016

Help us to manage our issues by answering the following:

  1. Describe your issue:

riot@3.0.0 breaks the code of rollup-plugin-riot@0.4.3 and rollup-plugin-commonjs

since riot@3.0.0 defines __esModule property, it is not compatible to some code.

eg:

import riot from 'riot'

the generated code with rollup-plugin-commonjs looks like:

function unwrapExports (x) {
	return x && x.__esModule ? x['default'] : x;
}

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

var riot_1 = createCommonjsModule(function (module, exports) {
     //riot code here...
});
var riot$1 = unwrapExports(riot_1);
//riot$1 is undefined after unwrapped

I've try to write code like:

 import * as riot from 'riot'

this time the generated code is:

function unwrapExports (x) {
	return x && x.__esModule ? x['default'] : x;
}

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

var riot_1 = createCommonjsModule(function (module, exports) {
     //riot code here...
});

var riot$1 = unwrapExports(riot_1);
//riot$1 is undefined after unwrapped

var riot$2 = Object.freeze({
	default: riot$1,
	__moduleExports: riot_1
});

neither of them work.

Solution:
riot module should export 'default' at the same time with __esModule to make it work.

  1. Can you reproduce the issue?

  2. On which browser/OS does the issue appear?

  3. Which version of Riot does it affect?
    3.0.0

  4. How would you tag this issue?

  • Question
  • [* ] Bug
  • Discussion
  • Feature request
  • Tip
  • Enhancement
  • Performance
@cognitom
Copy link
Member

@cognitom cognitom commented Nov 23, 2016

Hi @Joylei, rollup-plugin-riot@0.4.3 doesn't support Riot 3.0 yet. We're preparing v1.0, asap.

@Joylei
Copy link
Contributor Author

@Joylei Joylei commented Nov 23, 2016

@cognitom
I know that. even though rollup-plugin-riot support riot@3.0.0, it's still a issue with rollup-plugin-commonjs. From the generated code of rollup, you can see that two import methods are not working because the unwrapped module object is undefined.

import riot from 'riot';
import * as riot from 'riot';
@Rayraegah
Copy link

@Rayraegah Rayraegah commented Nov 23, 2016

In riot v3.0.0, using

import riot from 'riot'

throws error in riotjs-loader, webpack and webpack-dev-server environment.

changing this to

import * as riot from 'riot'

resolves the issue.

@cognitom cognitom mentioned this issue Nov 24, 2016
1 of 7 tasks complete
@cognitom
Copy link
Member

@cognitom cognitom commented Nov 24, 2016

I'm preparing a PR on this file.

Should we export everything with default? How about exporting only these core APIs:

  • riot.Tag
  • riot.tag
  • riot.mount
  • riot.mixin
  • riot.update
  • riot.unregister

I think this will be clearer and simpler.
If we need full APIs we could do that like this:

import riot, { settings, util } from 'riot'
import observable from 'riot-observable'

Any thought? @Joylei @Rayraegah

@Rayraegah
Copy link

@Rayraegah Rayraegah commented Nov 24, 2016

I like being able to import the bare minimum.

But this does introduce breaking changes when migrating from v2.x I've seen my developers import the entire riot package into stores just to use riot.observable rather than importing observable as a separate module.

@GianlucaGuarini
Copy link
Member

@GianlucaGuarini GianlucaGuarini commented Nov 24, 2016

@cognitom I would simply use something like this

@cognitom
Copy link
Member

@cognitom cognitom commented Nov 24, 2016

I'm considering about tree-shaking...

@GianlucaGuarini
Copy link
Member

@GianlucaGuarini GianlucaGuarini commented Nov 24, 2016

Here for example you import only the mount

@cognitom
Copy link
Member

@cognitom cognitom commented Nov 24, 2016

GianlucaGuarini added a commit that referenced this issue Nov 26, 2016
Fix #2084: export all as default
@GianlucaGuarini GianlucaGuarini added fixed and removed to verify labels Nov 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.