Added support for emitting System.register modules #2840
Conversation
…iable declarations are non-initialized
|
Would you recommend a commit-by-commit or files-changed review? I'm leaning towards the latter from what I can see. |
|
Our current testing of module formats is just to baseline both AMD and CommonJS all the time rather than needing to specify the module format for each test. Should we just start adding this as a 3rd default target? It's not necessarily high value (we don't exactly have a lot of tests that only break in AMD or CommonJS but not both) but it's nice not to have to deal with specifying it if the cost (test runtime, repo size) isn't that much worse. |
|
apparently this functionality was dropped at some point - now we always explicitly specify what kind of modules we would like to emit, i.e.here. However it seems to be a valuable thing to always produce baselines for all supported module kinds so we can automatically verify that emit is expected in all supported scenarios - I'm going to file a suggestion to bring this feature back. |
| * i.e non-exported variable statement 'var x = 1' is hoisted so | ||
| * we we emit variable statement 'var' should be dropped. | ||
| */ | ||
| function isSourceFileLevelDeclarationInSystemExternalModule(node: Node, isExported: boolean): boolean { |
There was a problem hiding this comment.
how about isSystemJSHoistableDeclaration or shouldHoist
There was a problem hiding this comment.
fixed, current name is shouldHoistDeclarationInSystemJsModule
| * if we should also export the value after its it changed | ||
| * - check if node is a source level declaration to emit it differently, | ||
| * i.e non-exported variable statement 'var x = 1' is hoisted so | ||
| * we we emit variable statement 'var' should be dropped. |
There was a problem hiding this comment.
can we just split them into two functions, one for hoistable declarations, and one for exported ones.
There was a problem hiding this comment.
I'll keep this function isSourceFileLevelDeclarationInSystemJsModule and add a helper named shouldHoistDeclarationInSystemJsModule that will call this function with isExported = false. There is only one place that invokes isSourceFileLevelDeclarationInSystemJsModule with isExported = true so I don't think it is worth adding one more helper just for this once case
There was a problem hiding this comment.
I really do not like the name. It is too long.
There was a problem hiding this comment.
what alternative do you propose?
|
👍 |
Added support for emitting System.register modules
Brief description: System.register explained