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

Properly export destructured export declarations in SystemJS #2587

Merged
merged 2 commits into from Dec 12, 2018

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Dec 8, 2018

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:
Closes #2556

Description

This will make sure variables defined via a destructuring assignment are exported when building SystemJS output. Example:

// input
export const {x, y: {z: a}, [u, v]} = someObject;
export const {w} = someOtherObject;

// output
const {x, y: {z: a}, [u, v]} = someObject; exports({x: x, a: a, u: u, v: v});
const {w} = someOtherObject; exports('w', w);

The exports are added inline with the declaration as otherwise the rendering logic would have become quite a bit more complex to get the indentation right.

Update: This will now also update exported variables which are reassigned in a non-declaration destructuring assignment via injecting an IIFE! Example:

// input
export let a, b;
console.log({a, b} = someObject);

// output
let a, b;
console.log(function (v) {exports({a: a, b: b}; return v;} ({a, b} = someObject));

As far as I can see, this transformation should always produce valid, though not 100% readable, code but please check if I missed something.

Copy link
Member

@TrySound TrySound left a comment

Choose a reason for hiding this comment

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

Great! Looks good to me

@lukastaegert
Copy link
Member Author

Thanks! Just after pushing the first version, I came up with a way to handle destructuring assignment expressions involving exported variables so this should hopefully solve this issue completely.

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Amazing work @lukastaegert thanks so much for this.

If you have any ideas re how this could be done more nicely in the format do let me know. We could for example add some other export variations if there is something that can help with this.

@lukastaegert
Copy link
Member Author

Well, I am not sure this would be useful to anyone except us and it might seem confusing why it exists, but one way to make it simpler could be if exports receives two arguments where the first one is not a string and the second one is an object , then it could use the second argument to update the exports but return the value of the first argument (without doing anything with it). Then we could just write:

// input
export let a, b;
console.log({a, b} = someObject);

// output
let a, b;
console.log(exports({a, b} = someObject, {a: a, b: b}));

JS semantics will make sure the first argument is evaluated before the second is evaluated. This could be the "inline expression form" of exports where you list the updates contained in an expression inline with an expression. Could also have a three argument version for single updates, e.g.

exports({a} = someObject, 'a', a)

@lukastaegert
Copy link
Member Author

Could also be an additional handler attached to exports so you do not have to check arguments, such as exports.inline(expression, {updates})

@guybedford
Copy link
Contributor

@lukastaegert good suggestions. I like the idea of switching the object form to take an expression first a lot.

There may well be a format update to use generators at some point (systemjs/systemjs#1826). If this happens we could update to use object-setters only like export([expr, ] updateObj) and deprecate export(name, value), as a new format mode.

@lukastaegert lukastaegert merged commit 371aa62 into master Dec 12, 2018
@lukastaegert lukastaegert deleted the system-export-destructuring branch December 12, 2018 05:44
@lukastaegert lukastaegert added this to In progress in Release 1.0.0 via automation Dec 16, 2018
@lukastaegert lukastaegert moved this from In progress to Done in Release 1.0.0 Dec 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Release 1.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

Destructured alias is not exported in system
3 participants