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

Add __generator helper to es6 source and populate jsnext:main #5

Merged
merged 7 commits into from
Nov 22, 2016

Conversation

frankwallis
Copy link
Contributor

This enables support for using the importHelpers option with module: 'es6' and then bundling the output with rollup, which automatically strips out any unused helper functions.

@frankwallis frankwallis changed the title Add __generator helper to es6 source and poopulate jsnext:main Add __generator helper to es6 source and populate jsnext:main Sep 30, 2016
@@ -27,5 +27,6 @@
"url": "https://github.com/Microsoft/tslib.git"
},
"main": "tslib.js",
"jsnext:main": "tslib.es6.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

how is this supposed to work on node versions that does not support const? i do not think rollup rewrites const to var?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that is a very good point, this issue seems to indicate that it should point to a file which is consumable by all engines for using es6 import/export statements.

In my scenario I actually do a second typescript transpilation to output the rollup bundle in system.register format and that removes the const keywords.

Shall I change the file to use export var?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to me that the ideal solution would be to have a single typescript source file which is then compiled to both targets, but I'm not sure how to handle the UMD / global wrapper stuff in the es5 file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the file to use var instead of const

@dmitrage
Copy link

dmitrage commented Oct 1, 2016

There is also __extends helper missing in tslib.es6.js. See my issue microsoft/TypeScript#9980.

Rollup also supports module field in package.json similar to jsnext:main.

Is there any reason why "export const" but not "export var" is used? All other code is ES5.

@frankwallis
Copy link
Contributor Author

@dmitrage I have just added the __extends helper as well. Will wait for feedback on the const/var issue.

@nathanhammond
Copy link

Ember CLI also would benefit from adding the __extends functionality into tslib.es6.js. We have a mild preference toward export var, but can use it in either way.

@imcotton
Copy link

imcotton commented Nov 5, 2016

/ping @mhegazy @rbuckton

This could be really useful, please have another look, thanks.

@@ -27,5 +27,6 @@
"url": "https://github.com/Microsoft/tslib.git"
},
"main": "tslib.js",
"jsnext:main": "tslib.es6.js",
Copy link

Choose a reason for hiding this comment

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

Maybe use module instead of jsnext:main to achieve same behavior, but less configuration since the module has been set to true as default in rollup-plugin-node-resolve.

@frankwallis
Copy link
Contributor Author

@imcotton, @dmitrage I have added the module field to package.json

# By Nathan Shively-Sanders (3) and others
# Via GitHub
* 'master' of https://github.com/Microsoft/tslib:
  Update version
  Fix microsoft#7: Update readme
  Update __rest emit to handle symbols
  Address PR comments.
  Add __rest helper
  Fix syntax error in __awaiter when running in ES3

# Conflicts:
#	tslib.es6.js
@frankwallis
Copy link
Contributor Author

@mhegazy - I have refreshed this PR with the latest changes, can it now be merged?

@mhegazy mhegazy merged commit faa3973 into microsoft:master Nov 22, 2016
@frankwallis
Copy link
Contributor Author

@mhegazy - thank you

@imcotton
Copy link

@mhegazy Thank you, btw could we anticipate that npm version includes this PR going to publish soon?

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

5 participants