Rollup optimization support

[guybedford]

For this plugin to support rollup optimizations, it should output ES modules directly instead of System.register modules when running in a build (var loader = this; if (this.builder == true && options.modules == 'system') options.modules = 'es6';).

[guybedford]

Note the above will only support SystemJS Builder 0.15 though.

[frankwallis]

I'm seeing an issue when I try this - when type-checking the plugin sets load.metadata.deps in translate in order to pull in any declaration (.d.ts) files which are needed for compilation. e.g.

load.metadata.deps = [ "file:/// ... /jspm_packages/npm/typescript/lib/lib.es6.d.ts!" ]

When I set the output to es6 these declaration files are no longer getting fed to the plugin and so it cannot type-check the files property. The issue also happens using the plugin-typescript -> plugin-babel -> es6 pipeline.

[guybedford]

@frankwallis metadata.deps is only supported for CommonJS, AMD and globals. Is it not possible to load these definition files manually?

[frankwallis]

It is possible but I want to put them in deps so that they will get reloaded by the hot-reloader when they change

[guybedford]

@frankwallis metadata.deps should be supported for System.register output actually, so that should be used in the browser. Perhaps you can fork this behaviour of typings as deps between browser and build environments? Strictly speaking as well, the typing shouldn't be a production dependency.

[frankwallis]

TBH it's a lot easier to let systemjs manage loading the files as it can take care of only loading them once and reloading them (when watching/hot-reloading). It also means bundle only gets called after they are all loaded which is nice.

The plugin translates these injected declaration files into empty files so they do not end up in the bundle, however I think perhaps the plugin should strip them out of the bundle in the bundle hook - is that possible?

Is there a technical reason that metadata.deps is not processed when outputting esm? Because if not I would rather address that than manually load these files! I also noticed that plugin-babel pushes files into metadata.deps in certain circumstances so there may be an issue there.

[guybedford]

@frankwallis even if they are empty files, that means their module names are still built into the bundle though surely? That doesn't sound like a good build technique to me unfortunately, although I understand the convenience. Stripping these modules from the bundle process is not an option as bundles are designed to retain dynamic linking semantics and only act as a transport multiplexing. These sorts of optimizations would break that.

The main reason for not allowing metadata deps for es modules is that it is not clear if this will even be possible in the loader spec. While the current spec would allow it via source code transformation (source = 'import "x"' + source), the ability to transform ES modules in the loader spec may turn out to be a feature that doesn't survive the next iteration, so I wouldn't want to create an ecosystem that relies on that.

This may well be an unnecessary assumption though, and depending how things go we could allow it in future, but the goal is to try and err on the side of conservatism to ensure the best chances of minimal pain going into a real loader.

Returning to your scenario, I would really suggest loading typings with a System.import blocking the plugin hooks (also to get the instance of System = this in the plugin hooks and not to use the global System to get the right instance). I'm still not sure I follow what is so hard about that?

[guybedford]

For the hot-reloading scenario are you saying that changing a typing file should change the related ts file, even if that file itself did not change? Is this not something that can be integrated into the __reload hook of the typing file itself through the plugin?

[frankwallis]

Thanks for the clarification, it is helpful for me to understand the bigger picture.

I think the issue was that previously I was using System.fetch to load the files manually, are you saying that I just need to issue a System.import on these files and they will get loaded by systemjs and fed through the plugin? That does seem pretty straightforward!

The plugin just wants to receive any updates to typings files which wasn't happening when using System.fetch, it sounds like this would not be a problem when using System.import as the files would be in the registry?

I am going to give this a try now.

[frankwallis]

@guybedford this seems to be working nicely, I have released 2.5.10 which uses System.import instead of load.metadata.deps. I still need to address the original issue of defaulting to es6 output when building though. Thanks for your help with this.

[guybedford]

Great to hear that. Is this definitely working with build support? Note that System.import doesn't actually execute, and requires System.pluginLoader.import in builds.

[frankwallis]

Yes it seems to work in all the scenarios, I'm just using the global System.import though. You are saying I should capture this in the hooks and use that instead of System?

https://github.com/frankwallis/plugin-typescript/blob/master/src/plugin.ts#L66

[guybedford]

A scoped System is provided for register modules so you're fine here, but if the plugin was CommonJS it wouldn't work.

Does it work for builds though? System.import should fail for builds with a builder error.

[frankwallis]

Yes, it seems to work for builds somehow.

[guybedford]

@frankwallis have you definitely tested it in the latest builder? It shouldn't work. Is there not supposed to be a handler on the promises in https://github.com/frankwallis/plugin-typescript/blob/master/src/plugin.ts#L66?

[frankwallis]

I am pretty certain it is working, but I will double-check later. We are not interested in the result of the System.import, but I suppose there ought to be a catch handler to trap errors.

[frankwallis]

I can confirm that System.import is definitely working for builds, using 0.15.7.
To repro:

git clone https://github.com/frankwallis/plugin-typescript.git
cd plugin-typescript
npm i
cd example/react
jspm i
npm run build

I have added the promise handler and the rollup support (the original issue) and it is released in plugin-typescript@3.0.0

[guybedford]

@frankwallis thanks I tried this out by adding a console.log to plugin.ts line 65 to see if it was running the System.import but didn't get that running on this example at all?

[frankwallis]

You need to add it to example/react/jspm_packages/github/frankwallis/plugin-typescript@version/lib/plugin.js (in the examples the plugin gets installed from github)

[frankwallis]

Also there is an issue using the rollup support - TypeScript cannot currently compile to es6 module format while outputting the code as es5. See microsoft/TypeScript#6319 which is tracking this.

This means that rollup support is only available when using plugin-babel in the pipeline. I think I need to revert the change which switches from system -> es6 in build mode until the above issue is resolved.

[guybedford]

Thanks @frankwallis I checked again and this is exactly right. Because you are writing the plugin as ES6 / System.register it is getting the right instance. This would not necessarily be true for a CommonJS or AMD plugin where we do not provide a scoped System, and the plugin should use this.import in the hooks.

It does sound like we're waiting on TypeScript for the Rollup support then. Will keep an eye on that issue as well.

[frankwallis]

@guybedford I'm seeing some issues using System.import to trigger the loading of files which would not be loaded by default. Initially the files I was importing were just typings which get transpiled to empty files and there were no problems. However I later discovered situations where I needed to System.import real typescript source files and this has highlighted a couple of problems:

1. When building with systemjs-builder the files are actually being executed and this can cause strange errors like window is undefined (see Attempt to upgrade to >=4.0.6 from 4.0.5: Curious ReferenceErrors regarding global variables #120). Is there a way I can trigger these files to be fed into the plugin without executing them?

2. If a file which I have imported with System.import('http://localhost/src/somefile.ts!http://jspm_packages/github/frankwallis/plugin-typescript/plugin.js') is later imported by another typescript file then it gets executed twice. It seems that the !http://jspm...plugin-typescript/plugin.js suffix means that the file is not recognised as the same file as http://localhost/src/somefile.ts and is held in the registry twice. This can easily be replicated by doing:

System.import('some-file.js');
System.import('some-file.js!some-loader.js');

will cause some-file.js to be executed twice, is that expected?

Would really appreciate your input here, thanks

[frankwallis]

One way I have found to get around (1) is to provide an empty instantiate hook, but I don't know what the impact of that will be in other scenarios.

[guybedford]

@frankwallis System.import within a plugin at build time is a live executing import function. When you output import 'x' in a transpiled source of a plugin, that will add dependencies that will not be executed. Would it be possible to inject import statements into the source instead of using the dynamic System.import? Let me know if that makes sense?

[frankwallis]

I would rather not do that into the actual files, but I could perhaps inject another file which just imports the files that I want, maybe using System.register or System.define.

would something like this work?:

var source = "import 'extra-file.ts!ts";
System.define(__SPECIAL_PLUGIN_FILE__, source);

[guybedford]

@frankwallis another thing you can do is to push dependencies into the metadata within the translate hook - load.metadata.deps.push(module). It's important to retain the conceptual model of each load simply having dependencies, which forms the full trace tree. Manually injecting loads does nothing if they are not branched properly into the tree. I can't recall if I've personally tested this case, so just let me know if there are any issues with that approach and we can fix them.

[frankwallis]

I was setting metadata.deps previously (see in February above), and that would be my ideal solution, but deps doesn't work when outputting to ES6 ("metadata.deps is only supported for CommonJS, AMD and globals")

[guybedford]

Right, thanks yes I remember now. This is entirely about being unsure if the spec will allow us to do it (it currently doesn't). If the spec doesn't allow this, then adding deps would basically be handled in SystemJS itself as just injecting import 'x' at the top of the source. But that doesn't make it impossible actually, so maybe its time to consider that - I've created systemjs/systemjs#1248.

Note that it would still be completely equivalent to doing that sort of injection of source = "import 'x';" + source, and that would be the workaround to get the same behaviour while waiting for such a thing to land.

[frankwallis]

Thanks, I have implemented as you suggested - metadata.deps is now set in all cases and when outputting esm it appends import "dep1";import "dep2"; to the source file. I will keep an eye on that issue, thanks for your help.

[guybedford]

Excellent, it sounds like the Rollup support should be working correctly then now? If so we can close this.

[frankwallis]

Unfortunately Rollup support is still waiting on microsoft/TypeScript#6319

[guybedford]

Looks like this is supported in microsoft/TypeScript#6319 now!

[guybedford]

All that should be needed here with the latest Typescript is something along the lines of:

if (this.builder) {
  typescriptOptions.module = "es2015";
  load.metadata.format = 'esm';
}
else {
  typescriptOptions.module = "system";
  load.metadata.format = 'register';
}

[frankwallis]

Rollup optimisation is available in plugin-typescript@5.0.6

[guybedford]

👍