Allow for system format modules to be named. Fixes #2027 by internettrans · Pull Request #2028 · rollup/rollup

internettrans · 2018-03-02T23:36:21Z

See #2027. This makes it possible to create named System.register bundles

internettrans · 2018-03-02T23:37:50Z

 const samples = path.resolve(__dirname, 'samples');

-const FORMATS = ['amd', 'cjs', 'es', 'iife', 'umd'];
+const FORMATS = ['amd', 'cjs', 'es', 'iife', 'umd', 'system'];


Let me know your thoughts on this -- the vast majority of the tests do not have an _expected/system.js file defined for the "form" tests. But I'm thinking that all future tests should.

This is included in the PR at #2022. Perhaps rebase to that if you want to do this?

internettrans · 2018-03-02T23:39:16Z

 			FORMATS.forEach(format => {
+				const skipBecauseNoSystem = format === 'system' && !fs.existsSync(path.resolve(__dirname, samples, dir, '_expected', 'system.js'))
+				if (skipBecauseNoSystem) {
+					return;


Don't call it() for "form" tests that don't have a system format expectation.

This prevents breaking existing tests but also providing a consistent way to add systemjs expectations in future tests.

This can also be avoided by rebasing to the other branch.

internettrans · 2018-03-02T23:39:40Z

+  description: 'names bundles correctly',
+  options: {
+    output: {
+      name: 'libraryName',


Before this pr, this name was ignored in the outputted System.register format.

internettrans · 2018-03-02T23:40:13Z

@@ -0,0 +1,10 @@
+System.register('libraryName', [], function (exports, module) {


Before my pr, 'libraryName', would not have appeared in this System.register call

guybedford

Thanks @joeldenning for the PR! Glad to hear you're exploring these workflows :)

The general approach seems good to me, with suggestion of rebasing to the system format test PR branch.

@Rich-Harris I would be interested to hear if there was any previous discussion on using the name option to refer to AMD module naming in output, as this seems related to that.

guybedford · 2018-03-03T15:02:22Z

Looking through the issue queue I can find nothing on named AMD defines, so assume it just hasn't been a needed use case. I take it code splitting is the feature that makes this different here really.

internettrans · 2018-03-06T02:27:58Z

Thanks @guybedford for the review -- I've rebased to master and have updated all the tests that previously were not naming their System.register bundles.

lukastaegert

Looks good from my side as well!

guybedford · 2018-03-06T12:49:41Z

It's worth considering here that this won't apply to the code splitting workflow very well.

It might be nice to think up a solution that can work with both single-file and code splitting workflows instead of having separate mechanisms for both of these.

Perhaps combining with the chunk naming feature would make more sense for code splitting integration - #1995. Just thinking aloud here though...

internettrans · 2018-03-06T19:52:50Z

@guybedford I'm not very familiar with rollup code splitting -- I heard it was announced pretty recently but don't know the mechanics of how it would work.

How do umd and iife handle named libraries and code splitting?

guybedford · 2018-03-07T11:52:42Z

UMD and IIFE output formats aren't actually supported for code splitting due to the lack of easy dependency modularity. I'm making progress on the general proposal for chunk naming. When such a feature lands, I tend to think that it would be a good fit for these named format outputs, potentially through a namedModules: true or similar boolean flag.

lukastaegert · 2018-03-14T05:25:17Z

@guybedford This PR is marked as requesting changes from you. Is this still the case or can this be merged?

guybedford · 2018-03-14T11:45:30Z

I still think it's worth discussing the API use cases here with regards to code splitting workflows a little further to ensure we have an approach that can work in both scenarios.

The new code splitting work will allow customizing all chunk names (in progress, I'm hoping to provide a PR shortly). If this is the case, it may well provide an API that can work for this feature being just a boolean flag.

In addition it may be worth making a separate top-level method for concatenation, something like generateCombined which only works for the system and amd formats (and in future if ES modules support named wrappers).

Then the single-file case becomes:

rollup.rollup({
  // allow input object to work in single-file case for naming:
  input: { 
    'inputName': 'input/file/path.js'
  }
}).generate({
  chunkNames: 'chunk-[hash].js',
  format: 'system',
  combineNamedModules: true
});

Which would output a single source containing System.register('inputName', [',/chunk-asdf897.js'], ...); System.register(',/chunk-asdf897.js', [], ...).

edit: This is kinda combining the code splitting and single-file workflows a bit... exact form still to be sketched out!

lukastaegert · 2018-03-14T12:00:16Z

Ok, so I'll leave this out of the current release.

internettrans · 2018-03-14T23:24:07Z

@guybedford since it sounds like code splitting is potentially going to be overhauled in the near future, would it make sense to address this problem at that time? The combineNamedModules option seems really cool (and relevant to #2036!), but it seems like there is some discussion and work to be done to decide if it will even happen.

Getting named system register modules at all would be cool -- I vote for merging this in and then iterating on it once the code splits / combineNamedModules discussions are fleshed out.

guybedford · 2018-03-16T16:31:27Z

@joeldenning I'm aiming to get a PR for chunk naming very soon... once that is in we can build this feature on top of that I think.

guybedford

@joeldenning after thinking about this a little further I actually think this is the best approach after all.

While a combineNamedModules option could work for code splitting alongside #2068, I'm not sure how beneficial it would be since chunk names are difficult to share between builds. In addition it still doesn't handle the single-file case anyway.

So it would only be the entry points that would be usefully shared... and running separate / multiple builds is likely a better technique here.

If you'd like to rebase this work I think it would be good to merge!

guybedford · 2018-03-21T11:24:51Z

I just noticed there is actually an amd.id output option. It's probably worth ensuring that whatever is done here either deprecates that or works with it then.

lukastaegert · 2018-03-24T22:17:33Z

 			`\n${t}${varOrConst} _starExcludes = { ${Array.from(starExcludes).join(': 1, ')}${starExcludes.size ? ': 1' : ''} };`;

 	const importBindingsSection = importBindings.length ? `\n${t}var ${importBindings.join(', ')};` : '';
+	const registeredName = outputOptions.name ? `'${outputOptions.name}', ` : '';


Looking through rollup's documentation, I think using name here is an abuse of this option which is meant to define global variable names for IIFE/umd bundles (maybe we want to add SystemJS to our umd output at some point?). As @guybedford pointed out, the amd.id option would be more equivalent except for the amd part of the name. So I would prefer deprecating amd.id in favour of a new output option. Not sure what the best name could be; we could call it something like moduleId as it is the id of the generated module as used by the loader. Maybe @guybedford has a more elaborate opinion on this.

guybedford · 2018-03-25T20:40:15Z

Yup, we'd need to avoid the name overload. moduleId as a new option to replace amd.id sounds really sensible to me actually, with it only supported in the non-code-splitting build. For the code splitting case, moduleId: true could possibly be an option as well I guess.

internettrans · 2018-03-27T23:49:37Z

@lukastaegert @guybedford I started work on switching over to moduleId instead of name, but discovered that moduleId is already a rollup output option that means something else. On top of that, it's a deprecated option, so I'm not sure if we would want to piggy back on it.

Maybe moduleName?

Also, would you like me to deprecate amd.id in favor of the new property? I think I know how to do so in the amd finalizer, but might need some help hunting down all the places where deprecation warnings are supposed to happen / documentation needs to be updated.

guybedford · 2018-03-28T08:21:45Z

It seems moduleId was exactly for this purpose but deprecated in 979b5e4.

At the time, AMD was the only module format with a moduleId, so I see the reasoning, but perhaps now with both AMD and System formats, it would be worth reconsidering this.

If we do undeprecate moduleId, then we should ensure that we provide warnings when moduleId is used with a format that is not AMD or System.

Personally I'd be for deprecating amd.id for going back to moduleId, but open to suggestions. @Rich-Harris @lukastaegert what do you think?

kerwitz · 2018-05-17T15:54:32Z

What's the status here?

CoskunSunali · 2018-05-18T23:07:04Z

Looking forward for this PR to be merged when ready. Can you guys be so kind to update us with a status?

lukastaegert · 2018-05-20T09:34:22Z

I think the only reason this has remained unmerged was the choice of option name. But I guess using name here should be ok so I'll merge this now.

lukastaegert · 2018-05-20T09:34:32Z

Sorry for the long wait!

lukastaegert · 2018-05-20T09:34:53Z

Will be a few days until we do another release, though.

CoskunSunali · 2018-05-20T13:37:27Z

🕺 Better late than never!

Thanks for the update.

Allow for system format modules to be named.

be90e75

internettrans commented Mar 2, 2018

View reviewed changes

guybedford requested changes Mar 3, 2018

View reviewed changes

Joel Denning added 2 commits March 5, 2018 13:23

Merge branch 'master' into named-system

1b74753

Updating tests

e7354bf

internettrans mentioned this pull request Mar 6, 2018

Bundle that calls System.register multiple times #2036

Closed

lukastaegert approved these changes Mar 6, 2018

View reviewed changes

lukastaegert added this to the 0.57.0 milestone Mar 6, 2018

lukastaegert removed this from the 0.57.0 milestone Mar 12, 2018

lukastaegert added this to the 0.57.0 milestone Mar 14, 2018

lukastaegert removed this from the 0.57.0 milestone Mar 14, 2018

guybedford approved these changes Mar 20, 2018

View reviewed changes

lukastaegert reviewed Mar 24, 2018

View reviewed changes

lukastaegert merged commit e7354bf into rollup:master May 20, 2018

lukastaegert modified the milestones: 0.59., 0.60.0 May 20, 2018

lukastaegert added the fixed pending release label May 20, 2018

lukastaegert removed the fixed pending release label Jun 6, 2018

jchip mentioned this pull request Jul 26, 2023

SystemJS named register with output.name support for code splitting #5074

Open

		@@ -0,0 +1,10 @@
		System.register('libraryName', [], function (exports, module) {

Uh oh!

Uh oh!

Conversation

internettrans commented Mar 2, 2018

Uh oh!

internettrans Mar 2, 2018

Choose a reason for hiding this comment

Uh oh!

guybedford Mar 3, 2018

Choose a reason for hiding this comment

Uh oh!

internettrans Mar 2, 2018

Choose a reason for hiding this comment

Uh oh!

guybedford Mar 3, 2018

Choose a reason for hiding this comment

Uh oh!

internettrans Mar 2, 2018

Choose a reason for hiding this comment

Uh oh!

internettrans Mar 2, 2018

Choose a reason for hiding this comment

Uh oh!

guybedford left a comment

Choose a reason for hiding this comment

Uh oh!

guybedford commented Mar 3, 2018

Uh oh!

internettrans commented Mar 6, 2018

Uh oh!

lukastaegert left a comment

Choose a reason for hiding this comment

Uh oh!

guybedford commented Mar 6, 2018

Uh oh!

internettrans commented Mar 6, 2018

Uh oh!

guybedford commented Mar 7, 2018

Uh oh!

lukastaegert commented Mar 14, 2018

Uh oh!

guybedford commented Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukastaegert commented Mar 14, 2018

Uh oh!

internettrans commented Mar 14, 2018

Uh oh!

guybedford commented Mar 16, 2018

Uh oh!

guybedford left a comment

Choose a reason for hiding this comment

Uh oh!

guybedford commented Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukastaegert Mar 24, 2018

Choose a reason for hiding this comment

Uh oh!

guybedford commented Mar 25, 2018

Uh oh!

internettrans commented Mar 27, 2018

Uh oh!

guybedford commented Mar 28, 2018

Uh oh!

kerwitz commented May 17, 2018

Uh oh!

CoskunSunali commented May 18, 2018

Uh oh!

lukastaegert commented May 20, 2018

Uh oh!

lukastaegert commented May 20, 2018

Uh oh!

lukastaegert commented May 20, 2018

Uh oh!

CoskunSunali commented May 20, 2018

Uh oh!

Reviewers

Assignees

Labels

guybedford commented Mar 14, 2018 •

edited

Loading

guybedford commented Mar 21, 2018 •

edited

Loading