System module format support by guybedford · Pull Request #1897 · rollup/rollup

guybedford · 2018-01-20T22:26:04Z

This provides support for the "system" module format option in Rollup.

It handles full live bindings support identical to the ES spec, including live binding reexport, export *, namespace import support, and dynamic import.

In the process of implementing I've brought through a RenderOptions argument to render as in #1882.

Because Rollup output is typically done around an application boundary for both single-file and chunk builds the support for hoisting functions through circular references is not provided in the Rollup output. I think this is an adequate tradeoff for the Rollup use cases in general and simplifies the output process nicely as well.

This will be great to test out some chunking workflows with minimal SystemJS builds - so I'd really love if it's possible to land this alongside the chunking work, but let me know what we have time for.

guybedford · 2018-01-20T22:30:28Z

For now the tests run only against the chunking form tests. These can be expanded fine, just will be a bit of a process.

kellyselden · 2018-01-20T23:08:38Z

Can we land #1882 first? Then we can all use it as a base.

guybedford · 2018-01-20T23:25:06Z

@kellyselden this exactly implements the approach in #1882 already, against the code splitting branch as well so you don't need to go through the rebase on that.

kellyselden · 2018-01-20T23:32:28Z

If it landed in master first, it would be easier for me to rebase, rather than the entirety of code-splitting.

guybedford · 2018-01-20T23:39:11Z

@kellyselden unfortunately because the es property is removed in code splitting, it won't affect your diff regardless.

kellyselden · 2018-01-20T23:45:39Z

I don't follow. If #1882 merges to master, and #1878 rebases on top of it, then I no longer touch every render line, meaning your branch with all the es removals would not affect me as much.

guybedford · 2018-01-21T00:03:09Z

@kellyselden when you asked before about moving forward I suggested rebasing your work to the code splitting PR as a lot of the finalizer stuff and rendering is affected by that. It sounds to me like you are now asking me (indirectly as well?) to rebase the code splitting work to your PR?

kellyselden · 2018-01-21T00:14:52Z

Not the entirety of #1878. I just figured since we both want to use the code in #1882, it would make sense to get it in master first so we can both rebase on top of it.

guybedford · 2018-01-21T00:18:45Z

@kellyselden how about this - if this PR is delayed for any reason after the code splitting PR lands, we can rebase out the RenderOptions part on top of code splitting directly if it makes your diffs easier. But I'm really hoping this can land alongside the code splitting which will hopefully be in the next day or two.

lukastaegert

I'm ok with most of this and agree that this is a HUGE addition as it would basically give as a runtime we can use for older browsers (which leaves us on par with e.g. Webpack in this respect but with noticeably smaller bundles as we never replicate modules—very nice).

I think we definitely need a test for the "starExcludes" case. Also, the need for functional tests of the chunking logic is becoming more and more important as with all the bits of code being inserted here, actually running it seems to be a better guarantee that everything is working than checking "on sight" (cf. "starExcludes" vs. "_starExcludes"). Not sure how much work this will be for the "system" case, though.

As I did no even manage to go through everything @kellyselden submitted, I will postpone the release at least until tomorrow or maybe Wednesday.

lukastaegert · 2018-01-22T05:57:35Z

-	private setDynamicImportResolutions (format: string) {
-		const es = format === 'es';
-		let dynamicImportMechanism: DynamicImportMechanism;
+	private setDynamicImportResolutions (options: OutputOptions) {


({format}: OutputOptions) would reduce the noise further down. Or just leave it as it is?

lukastaegert · 2018-01-22T06:02:18Z

 	initialise (parentScope: Scope): void;
 	initialiseAndDeclare (parentScope: Scope, kind: string, init: ExpressionEntity | null): void;
-	render(code: MagicString): void;
+	render(code: MagicString, opts: RenderOptions): void;


If you just call it options instead of opts, it will certainly reduce merge conflicts for @kellyselden. No need for unnecessary abbreviations here.

(Also, I'm not a huge fan of abbreviations. What seems obvious to yourself at some point does not always seem obvious to everyone else every time especially when code is read out of context. opts is probably not easily misunderstood but it sets precedence for a code style that uses more abbreviations which I am not in favour of due to the reasons outlined above. As you said yourself at some point, people tend to copy the code style they find.)

lukastaegert · 2018-01-22T06:28:17Z

+
+		const renderOptions: RenderOptions = {
+			systemBindings: system,
+			importMechanism: undefined


Maybe

importMechanism: this.graph.dynamicImport ? this.setDynamicImportResolutions(options) : undefined

or even

importMechanism: this.graph.dynamicImport && this.setDynamicImportResolutions(options)

and removing the next if statement would reduce clutter here

lukastaegert · 2018-01-22T06:38:29Z

+) {
+	const { dependencies, exports } = chunk.getModuleDeclarations();
+
+	const deps = dependencies.map(m => `'${getPath(m.id)}'`);


deps are actually dependencyIds or dependencyPaths. Would help readability to have a more descriptive name especially since it is only used once much further down.

Also clashes with the fact that dep further down is not a single item of deps 😉

lukastaegert · 2018-01-22T07:10:39Z

+						if (starExcludes.indexOf('default') === -1)
+							starExcludes.push('default');
+						// also include reexport names
+						dependencies.forEach(dep => {


This is unfortunate naming as dep here shadows dep in the upper context (which is the kind of thing I hope at some point we can have a linter point out) which hurts readability.

As this block is only executed once and otherwise clutters the loop, I would suggestExtracting it e.g. as a getStarExcludes(exports, dependencies) which will also solve the naming conflict and again make the really long function a little more readable 😉

Also it might be worthwhile to consider making starExcludes e.g. a Set which will make all costly .indexOf checks obsolete and can easily be constructed from any array via new Set(someArray) (also being a native data structure, Sets are quite optimised for access). Of course in the end, you would need to write something like Array.from(starExcludes).join(...) and you would loose guaranteed order which in this case does not seem important (but I may be wrong).

Or of course using an object + Object.keys

lukastaegert · 2018-01-22T07:28:23Z

+						});
+					}
+					setter.push(`for (var _$p in module) {`);
+					setter.push(`${t}if (!starExcludes[_$p]) _setter[_$p] = module[_$p];`);


Also, there is no single test that ever triggers this code. Which is unfortunate as I have the feeling that e.g. starExcludes here should rather be _starExcludes? Might also be an interesting test case for the other formats.

Good catch. I've included a test for this now.

lukastaegert · 2018-01-22T07:32:23Z

+          fn$1();
+          fn();
+        }
+      } exports('default', Main1);


Wouldn't this look better if exports was on a new line? It seems to be valid syntax but it sure looks strange.

The issue I found with this is that adding a new line results in the indentation method of MagicString not applying properly, and I didn't want to get into fixing that. I also thought this seems like some kind of nice shorthand anyway.

guybedford · 2018-01-22T15:08:37Z

@lukastaegert thanks for the quick review, I've included all the changes and caught an important bug on the reexport testing.

As for function tests, ideally yes we should have the SystemJS loading running (which I have been testing here), but I was hesitant to add SystemJS as a dependency just yet for rollup. Will aim to follow up here with more tests. Currently I'm also aiming to get a SystemJS release out at the same time with a 6KB production loader that can work with these use cases while supporting CDN externals of any format (AMD / UMD / System / Global). Will be good to get that on the tests when it's ready I think.

guybedford mentioned this pull request Jan 20, 2018

SystemJS output #83

Closed

guybedford force-pushed the system-format branch 3 times, most recently from e2df971 to 5d13946 Compare January 21, 2018 12:40

lukastaegert reviewed Jan 22, 2018

View reviewed changes

lukastaegert added this to the 0.55.0 milestone Jan 22, 2018

lukastaegert changed the base branch from code-splitting-rebased to release-0.55.0 January 22, 2018 07:42

guybedford force-pushed the system-format branch from 5d13946 to a0aea38 Compare January 22, 2018 15:06

guybedford added 2 commits January 22, 2018 17:44

system module format integration

e2ce49b

feedback, reexport testing and bug fixes

b8b3038

guybedford force-pushed the system-format branch from a0aea38 to b8b3038 Compare January 22, 2018 15:45

lukastaegert approved these changes Jan 23, 2018

View reviewed changes

lukastaegert merged commit b8b3038 into release-0.55.0 Jan 23, 2018

guybedford deleted the system-format branch January 23, 2018 15:23

Uh oh!

Uh oh!

Conversation

guybedford commented Jan 20, 2018

Uh oh!

guybedford commented Jan 20, 2018

Uh oh!

kellyselden commented Jan 20, 2018

Uh oh!

guybedford commented Jan 20, 2018

Uh oh!

kellyselden commented Jan 20, 2018

Uh oh!

guybedford commented Jan 20, 2018

Uh oh!

kellyselden commented Jan 20, 2018

Uh oh!

guybedford commented Jan 21, 2018

Uh oh!

kellyselden commented Jan 21, 2018

Uh oh!

guybedford commented Jan 21, 2018

Uh oh!

lukastaegert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guybedford commented Jan 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants