Skip to content

Set __moduleName from context.id argument#6688

Merged
mhegazy merged 1 commit into
microsoft:masterfrom
guybedford:system-module-id
Jan 29, 2016
Merged

Set __moduleName from context.id argument#6688
mhegazy merged 1 commit into
microsoft:masterfrom
guybedford:system-module-id

Conversation

@guybedford
Copy link
Copy Markdown
Contributor

@guybedford guybedford commented Jan 28, 2016

This resolves #6359 using the method suggested by @vladima as the replacement to the original PR at #6098.

This context object is described at https://github.com/ModuleLoader/es6-module-loader/wiki/System.register-Explained#how-it-works, and was considered more extensible for future spec changes than a string second argument directly.

@msftclas
Copy link
Copy Markdown

msftclas commented Jan 28, 2016

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

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@guybedford guybedford mentioned this pull request Jan 28, 2016
Closed
@msftclas
Copy link
Copy Markdown

msftclas commented Jan 28, 2016

@guybedford, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

@mhegazy
Copy link
Copy Markdown
Contributor

mhegazy commented Jan 28, 2016

👍, looks like we have two unit tests that need to be updated.
@vladima, can you take a look.

@vladima
Copy link
Copy Markdown
Contributor

vladima commented Jan 28, 2016

I think this variable has to be injected after the prologue directive "use strict"

mhegazy
mhegazy reviewed Jan 28, 2016
View reviewed changes
Comment thread src/compiler/emitter.ts Outdated
Copy link
Copy Markdown
Contributor

@mhegazy mhegazy Jan 28, 2016

Choose a reason for hiding this comment

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

this should be var __moduleName = ${contextObjectForFile} && ${contextObjectForFile}.id; to avoid breaking users of older JSPM versions.

@guybedford
Copy link
Copy Markdown
Contributor Author

guybedford commented Jan 29, 2016

Thanks, I've made these corrections.

@vladima
Copy link
Copy Markdown
Contributor

vladima commented Jan 29, 2016

@guybedford can you please fix two remaining failing tests so we can pull in this PR?

@guybedford
Copy link
Copy Markdown
Contributor Author

guybedford commented Jan 29, 2016

@vladima sure I've got them passing now.

mhegazy added a commit that referenced this pull request Jan 29, 2016
@mhegazy
Merge pull request #6688 from guybedford/system-module-id
1ae7d5c 
Set __moduleName from context.id argument
@mhegazy mhegazy merged commit 1ae7d5c into microsoft:master Jan 29, 2016
@mhegazy
Copy link
Copy Markdown
Contributor

mhegazy commented Jan 29, 2016

thanks!

@mhegazy mhegazy added this to the TypeScript 1.8 milestone Jan 29, 2016
@mhegazy mhegazy added the Breaking Change Would introduce errors in existing code label Jan 29, 2016
@mhegazy
Copy link
Copy Markdown
Contributor

mhegazy commented Jan 29, 2016

@vladima can you merge this in release-1.8 as well.

vladima pushed a commit that referenced this pull request Jan 29, 2016
@mhegazy @vladima
Merge pull request #6688 from guybedford/system-module-id
3bfc6a0 
Set __moduleName from context.id argument
@vladima vladima mentioned this pull request Jan 29, 2016
Merged
vladima added a commit that referenced this pull request Jan 29, 2016
@vladima
Merge pull request #6729 from Microsoft/port-6688
f3ad8bf 
Ports #6688 in release-1.8
@guybedford
Copy link
Copy Markdown
Contributor Author

guybedford commented Jan 29, 2016

👍 Thanks for all the help getting this in.

@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.

Reviewers

No reviews

Assignees

No one assigned

Labels

Breaking Change Would introduce errors in existing code

Projects

None yet

Milestone

TypeScript 1.8

Development

Successfully merging this pull request may close these issues.

System.register module name update

4 participants

@guybedford @msftclas @mhegazy @vladima