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

Fix #2084: export all as default #2091

Merged
merged 1 commit into from Nov 26, 2016

Conversation

Projects
None yet
2 participants
@cognitom
Member

cognitom commented Nov 24, 2016

1. Have you added test(s) for your patch? If not, why not?

I could add a test later. Or we could merge this soon as a hot fix for ES6 users, I think. This change has no effect for existing codes.

Content

See #2084

@cognitom

This comment has been minimized.

Show comment
Hide comment
@cognitom

cognitom Nov 25, 2016

Member

I think it will close this issue, too.
riot/rollup-plugin-riot#1

Member

cognitom commented Nov 25, 2016

I think it will close this issue, too.
riot/rollup-plugin-riot#1

@GianlucaGuarini GianlucaGuarini merged commit 2fc1934 into dev Nov 26, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Nov 26, 2016

Member

thanks @cognitom probably in future we will need to integrate the export default directly in riot.js avoiding an extra file only for the es6 exports

Member

GianlucaGuarini commented Nov 26, 2016

thanks @cognitom probably in future we will need to integrate the export default directly in riot.js avoiding an extra file only for the es6 exports

@cognitom

This comment has been minimized.

Show comment
Hide comment
@cognitom

cognitom Nov 26, 2016

Member

@GianlucaGuarini thanks!

Yeah, I hope so, too. I made another file index.js because of a issue below.
Bundlers like Rollup doesn't support the hybrid approach. For example, in this case, const c would be exported in ES6 syntax:

export const a = 'apple'
export const b = 'banana'
export const c = 'coffee'
export default {a, b}

But not in the compiled CommonJS code:

module.exports = {
  a: 'apple',
  b: 'banana'
}

Because default has priority than named exports.

So we need to carefully sync named exports and its default always. The point of this PR is that there's no need to sync them manually, I think.

Member

cognitom commented Nov 26, 2016

@GianlucaGuarini thanks!

Yeah, I hope so, too. I made another file index.js because of a issue below.
Bundlers like Rollup doesn't support the hybrid approach. For example, in this case, const c would be exported in ES6 syntax:

export const a = 'apple'
export const b = 'banana'
export const c = 'coffee'
export default {a, b}

But not in the compiled CommonJS code:

module.exports = {
  a: 'apple',
  b: 'banana'
}

Because default has priority than named exports.

So we need to carefully sync named exports and its default always. The point of this PR is that there's no need to sync them manually, I think.

@GianlucaGuarini GianlucaGuarini referenced this pull request Nov 26, 2016

Closed

Add hot reloading #2095

@cognitom cognitom referenced this pull request Dec 7, 2016

Closed

import riot from 'riot' exports undefined (with webpack) #2138

1 of 7 tasks complete

@GianlucaGuarini GianlucaGuarini deleted the fix/export-default branch Jan 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment