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

Naive implementation for #6097 #6098

Merged
merged 2 commits into from Jan 4, 2016
Merged

Naive implementation for #6097 #6098

merged 2 commits into from Jan 4, 2016

Conversation

guybedford
Copy link
Contributor

As in #6097 this adds System.register __moduleName reference support.

An optimization that can be included is to only add this argument when it is explicitly used in the code, making the output cleaner, but it would be removed by minification anyway if it is always output.

@msftclas
Copy link

Hi @guybedford, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. Real humans will now evaluate your PR.

TTYL, MSBOT;

@vladima
Copy link
Contributor

vladima commented Dec 14, 2015

the change per se looks good, however can you please also update test baselines so we can see overall impact?
For compiler tests this can be done via

# do a clean build and run tests
jake clean local runtests
# accept current test results as baselines
jake baseline-accept

Unittests baselines should be updated in code.

Also since this will introduce new intrinsic name that user cannot rename we probably should either add a collision detection logic to make sure that there will be no identifiers named __module in user code similar to what we do for _arguments and _this today or expose some way for a user to pick a name for this second parameter.

@guybedford
Copy link
Contributor Author

@vladima sure, I've updated the baselines and pushed that through. If going the collision detection route it might be nice to also only add the reference when it is already a free global __moduleName reference in the existing code.

@vladima
Copy link
Contributor

vladima commented Dec 14, 2015

thanks! can you please also update baselines in transpile set suite: https://github.com/Microsoft/TypeScript/blob/master/tests/cases/unittests/transpile.ts ?

@guybedford
Copy link
Contributor Author

Sure, no problem... it looks like the tests are passing now.

@vladima
Copy link
Contributor

vladima commented Dec 14, 2015

@guybedford great, thanks!

@DanielRosenwasser
Copy link
Member

@vladima is that a sign off? Is this ready to go in?

@DanielRosenwasser
Copy link
Member

Discussed offline; one current problem is that we'll never report an error if a user defines their own __moduleName. I'd prefer if we had a solution that protects the user, but I can't definitely commit until @mhegazy returns.

@guybedford
Copy link
Contributor Author

There isn't any actual conflict - if a user chooses to redefine __moduleName they can. Better tracking of this variable use in the implementation is definitely something to consider though.

@mhegazy
Copy link
Contributor

mhegazy commented Jan 4, 2016

👍

@mhegazy
Copy link
Contributor

mhegazy commented Jan 4, 2016

thanks @guybedford!

mhegazy added a commit that referenced this pull request Jan 4, 2016
@mhegazy mhegazy merged commit 996f169 into microsoft:master Jan 4, 2016
vladima added a commit that referenced this pull request Jan 5, 2016
This reverts commit 996f169, reversing
changes made to 51fd41b.
vladima added a commit that referenced this pull request Jan 5, 2016
Revert "Merge pull request #6098 from guybedford/master"
johnjbarton pushed a commit to google/traceur-compiler that referenced this pull request Jan 6, 2016
Thanks so much for the previous work in removing the `System` global dependency. I'm in the process of finalising the new Traceur plugin for SystemJS and will report back further if there are any further hitches.

I just wanted to go back to this format adjustment in System.register before it is too late though.

While discussing with @sebmck in the related Babel PR, it seemed a context object might be more appropriate as it can the accommodate and expand to fill whatever role the ES Module contextual syntax uses in future.

I'd be interested to hear both your thoughts on this format adjustment.

The PR here updates Traceur to use this idea of a context object argument with just one `id` property for now, which may well be a much more adaptable approach.

This is in line with the updated Babel PR at babel/babel#3166 and I will also see if I can update the TypeScript PR at microsoft/TypeScript#6098 as well.

Review URL: https://api.github.com/repos/google/traceur-compiler/issues/2051

Closes #2051.
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants