search

google / traceur-compiler

Traceur is a JavaScript.next-to-JavaScript-of-today compiler
Apache License 2.0
8.17k stars 578 forks source link

Use a context argument in System.register #2051

Closed guybedford closed 8 years ago

guybedford commented 8 years ago

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 https://github.com/babel/babel/pull/3166 and I will also see if I can update the TypeScript PR at https://github.com/Microsoft/TypeScript/pull/6098 as well.

johnjbarton commented 8 years ago

My understanding is that your proposal here is "Replace the moduleName module-environment string value with an object named $context, initially containing one property keyed 'id' and having the _moduleName value".

My only concern is the name of the object. Using 'context' gives us no hints and searching online or through source code would lead down many blind alleys. Would you consider say "module" so the change would be __moduleName" -> "$__module.id?

($__ is also overused, to avoid collisions I guess say 16_module would work better.).

guybedford commented 8 years ago

How about $__moduleContext then?

guybedford commented 8 years ago

And yes your summary is exactly correct!

johnjbarton commented 8 years ago

The Context in moduleContext seems redundant, but if you think its clear then its fine by me.

arv commented 8 years ago

__moduleContext seems fine to me. The name is a placeholder and making it more explicit even though it is redundant makes it even more clear that that is the case.

arv commented 8 years ago

LGTM with nits.

@johnjbarton Can you merge if it lgty?

guybedford commented 8 years ago

Thanks, corrections made.

johnjbarton commented 8 years ago

I'm confused by the diff, @guybedford please check that github has the PR the way you want it.

guybedford commented 8 years ago

@johnjbarton I've updated to include the comment as you suggested. Otherwise the diff looks good to me.

johnjbarton commented 8 years ago

I was expecting to see $__moduleContext, or perhaps a reason you wanted to stick with $__context.

guybedford commented 8 years ago

Ah right! Sorry I completely forgot, I've added that in now.

guybedford commented 8 years ago

:+1: thanks for the quick review.

tbragaf commented 8 years ago

Hi all!

Traceur is parsing moduleName to $moduleContext.id right? Because in my transpiled code, moduleName has been parsed to $moduleContext.id. I read about the change.

But the problem is $moduleContext itself is a string with moduleName value, so $__moduleContext.id undefined... What is exactly happening here?

However, using __moduleName is just a matter of time before it disappears, correct? How can correctly handles this now? I am using ES6 modules.

Thank you! tbragaf