build: import tslib instead of letting tsc generate helpers #4787
Conversation
googlebot
added
the
cla: yes
jelbourn
force-pushed
the
tslib
branch
2 times, most recently
from
0f23592 to
c02284e
Compare
| bundleOptions.plugins = [rollupNodeResolutionPlugin()]; | ||
|
|
||
| // Clone ROLLUP_GLOBALS so we don't accidentally modify it. | ||
| bundleOptions.globals = Object.assign({}, ROLLUP_GLOBALS); |
There was a problem hiding this comment.
Do we want to change writeOptions.globals?
There was a problem hiding this comment.
Good catch, I don't think this part is doing anything at all (and isn't needed)
| @@ -67,5 +72,19 @@ export function createRollupBundle(config: BundleConfig): Promise<any> { | |||
| sourceMap: true | |||
| }; | |||
|
|
|||
| // When creating a UMD, we want to exclude tslib from the `external` and `globals` bundle options | |||
| // so that it is inlined into the bundle. | |||
| if (config.format === 'umd') { | |||
There was a problem hiding this comment.
I think we should rather add a new option to the BundleConfig that says includeTslib
There was a problem hiding this comment.
I think that would be redundant, though, since we only want to do this for UMDs anyway
There was a problem hiding this comment.
It's just too much magic IMO. The rollup-helpers is a helper file that should be at least a bit generic.
Especially when we expose the package utilities (to for example flex-layout)
There was a problem hiding this comment.
We would want the same behavior for flex-layout (and anything else we build). The UMD really is the special case.
There was a problem hiding this comment.
That was just an example. At some point it will end up similar as with the convertIndexShorthandImports in tsc-wrapped.
It's basically the same question about just inferring the dest path on our own. Being flexible just opens more doors and is less like a black-box.
Anyway, your decision. I don't like it to be honest and this makes the helper into something more magic.
| // When creating a UMD, we want to exclude tslib from the `external` and `globals` bundle options | ||
| // so that it is inlined into the bundle. | ||
| if (config.format === 'umd') { | ||
| bundleOptions.plugins = [rollupNodeResolutionPlugin()]; |
There was a problem hiding this comment.
In theory we could always add this plugin. It shouldn't change anything for normal FESM bundles.
There was a problem hiding this comment.
I'd rather leave it where it's needed rather than just installing it all the time
|
LGTM |
jelbourn
added
action: merge
Recently with angular#4787 the tsc generated helpers got replaced with helpers from the `tslib` package. This means that the generated IIFE statements are different than before and the pure annotations RegExps don't work anymore.
Recently with #4787 the tsc generated helpers got replaced with helpers from the `tslib` package. This means that the generated IIFE statements are different than before and the pure annotations RegExps don't work anymore.
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
CC @IgorMinar