feat: dynamic rollup configuration for esm flattening #395

alan-agius4 · 2017-12-11T10:41:48Z

I'm submitting a...

[ ] Bug Fix
[X] Feature
[ ] Other (Refactoring, Added tests, Documentation, ...)

Checklist

Commit Messages follow the Conventional Commits pattern
- A feature commit message is prefixed "feat:"
- A bugfix commit message is prefixed "fix:"
Tests for the changes have been added

Description

externals has been removed in favor of umdModuleIds which is now used to just provide the UMD module IDs of external libraries (in case they cannot be auto-guessed by rollup; see further below).

By default all dependencies are now treated as externals and thus are not embedded in the final bundle.

However, if you want to embed one or more of them into the final bundle you can do so by adding the external dependency in the embedded section like so:

{
  "$schema": "../../../src/ng-package.schema.json",
  "lib": {
    "embedded": [
      "lodash",
      "date-fns"
    ]
  }
}

While rollup will do it's best to guess the UMD module ID of an external dependency (and ng-packagr provides common defaults for angular and rxjs) some modules are hard to guess and it's recommended to provide the name of the external dependency. This can be done by using umdModuleIds in the config section like so:

{
  "$schema": "../../../src/ng-package.schema.json",
  "lib": {
    "umdModuleIds": {
      "lodash" : "_",
      "date-fns" : "DateFns",
    }
  }
}

A migration example can be found in ab52732

Does this PR introduce a breaking change?

[X] Yes
[ ] No

Closes: #312

alan-agius4 · 2017-12-11T13:14:33Z

src/lib/steps/rollup.ts

-    ...opts.externals,
+  const embedded: string[] = opts.embedded || [];
+  const namedExternals = {
+    'tslib': 'tslib',


might also make sense to add some other defaults that are widely used example, ionic-angular, lodash, moment, date-fns, react etc..

alan-agius4 · 2017-12-11T20:26:33Z

Need to update lockfile, though I am on my mobile at the moment!

dherges · 2017-12-11T20:30:19Z

src/lib/steps/rollup.ts

+        embedded.some(x => x === moduleId)
+      ) {
+        // if it's either 'absolute', marked to embed, starts with a '.' or '/' it's not external
+        return false;


What do you think of extracting to a function / lambda?

const externalModuleIdStrategy = (moduleId: string): boolean => { /* ... */ }

Then write a unit test for it?

I’d love that, if you tell me where to place the unit test etc... :) I’d find it much more easy to test.

Small correction from your example, to test it, you need to export it though!

Ah, yes! 😄

dherges · 2017-12-11T20:32:31Z

src/lib/steps/rollup.ts

+      }
+
+      return namedExternals[moduleId] || ''; // leave it up to rollup to guess the global name
+    },


See above. Extract to a function / named lambda and then write a unit test for that piece of functionality?

const umdModuleIdStrategy = (moduleId: string): string => { /* ... */ } /* ... */ describe(`umdModuleIdStrategy`, () => { it(`should map rxjs/add/operator/<foo> to Rx.Observable.<foo>`, () => { /* ... */ }); });

???

Yeah, I agree.

dherges · 2017-12-11T20:43:51Z

Can you try to reset to origin/master? Ideally it should be enough to keep the changes to these files:

Unit tests have been added on the master, command: yarn test:specs

The test file is named <being-tested>.spec.ts. Keep that pattern with rollup.ts and rollup.spec.ts.

dherges · 2017-12-11T20:45:44Z

Here's an example:

https://github.com/dherges/ng-packagr/blob/0079deb38f64320ab71d02d3f4c0056c5bf08ca9/src/lib/util/ts-transformers.spec.ts#L1-L9

alan-agius4 · 2017-12-11T20:46:32Z

Thanks, will do that.

alan-agius4 · 2017-12-12T07:32:30Z

@dherges Unit tests added, though I found a small issue #402

dherges · 2017-12-17T15:37:31Z

From rollup's API docs:

globals: Object of id: name pairs, used for umd/iife bundles.

Let's rename namedExternals toumdModuleIds since it's only intention is for helping rollup to find umd module ids.

dherges · 2017-12-17T15:41:38Z

src/lib/steps/rollup.spec.ts

+      expect(umdModuleIdStrategy('foo')).to.empty;
+    });
+  });
+});


dherges · 2017-12-17T15:46:59Z

integration/samples/rollup/README.md

@@ -0,0 +1,2 @@
+Sample library: external and embeded dependencies 
+=====================================


Ok, I think we can move this integration test to integration/samples/external

dherges · 2017-12-17T15:47:29Z

@alan-agius4 When you make these two little adjustments, I think it's good to go!

BREAKING CHANGE: `externals` has been removed in favor of `umdModuleIds` which is now only used to name the external libraries. By default all dependencies are now treated as externals and thus are not embedded in the final bundle. However, If you want to embed one or more of them into the final bundle you can do so by adding the library in the `embeeded` section like so; ```json { "$schema": "../../../src/ng-package.schema.json", "lib": { "embedded": [ "lodash", "date-fns" ] } } ``` While `rollup` will do it's best to guess the name of an external library some modules are hard to guess and it's recommended to provide the name of the external dependency. This can be done by using `umdModuleIds` in the config section like so; ```json { "$schema": "../../../src/ng-package.schema.json", "lib": { "umdModuleIds": { "lodash" : "_", "date-fns" : "DateFns", } } } ``` Closes: #312

alan-agius4 · 2017-12-17T20:28:07Z

@dherges updated as requested.

dherges · 2017-12-19T18:07:17Z

@alan-agius4 perfect!

dherges · 2017-12-19T18:23:49Z

Just added a few words to the readme, hoping to clarify and be precise on the terms "externals", "dependency", "bundle", and so on

dherges · 2017-12-19T18:24:06Z

Thank you Alan, this is a good step to the right direction!

alan-agius4 · 2017-12-19T18:52:14Z

Glad to help :)

github-actions · 2020-06-19T16:09:06Z

This PR has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

_{This action has been performed automatically by a bot.}

alan-agius4 commented Dec 11, 2017

View reviewed changes

dherges changed the title ~~Feature/dynamic rollup~~ feat: dynamic rollup configuration for esm flattening Dec 11, 2017

dherges reviewed Dec 11, 2017

View reviewed changes

dherges added the roadmap (high) label Dec 11, 2017

dherges reviewed Dec 17, 2017

View reviewed changes

src/lib/steps/rollup.spec.ts Outdated

expect(umdModuleIdStrategy('foo')).to.empty;

});

});

});

Copy link

Contributor

dherges Dec 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

dherges reviewed Dec 17, 2017

View reviewed changes

docs: clarify some wording / typoes

5e7502f

dherges merged commit 5712429 into ng-packagr:master Dec 19, 2017

alan-agius4 deleted the feature/dynamic-rollup branch December 19, 2017 18:23

alan-agius4 mentioned this pull request Dec 19, 2017

Include commonjs module in the bundle #423

Closed

dherges removed the roadmap (high) label Jan 10, 2018

pfeigl mentioned this pull request Jan 25, 2018

"Cannot create property 'modifierFlagsCache'" when using string-based enums #206

Closed

github-actions bot locked and limited conversation to collaborators Jun 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: dynamic rollup configuration for esm flattening #395

feat: dynamic rollup configuration for esm flattening #395

alan-agius4 commented Dec 11, 2017 •

edited by dherges

Loading

alan-agius4 Dec 11, 2017

alan-agius4 commented Dec 11, 2017

dherges Dec 11, 2017

alan-agius4 Dec 11, 2017

alan-agius4 Dec 11, 2017

dherges Dec 11, 2017

dherges Dec 11, 2017

dherges Dec 11, 2017

alan-agius4 Dec 11, 2017

dherges commented Dec 11, 2017

dherges commented Dec 11, 2017

alan-agius4 commented Dec 11, 2017

alan-agius4 commented Dec 12, 2017 •

edited

Loading

dherges commented Dec 17, 2017

dherges Dec 17, 2017

dherges Dec 17, 2017

dherges commented Dec 17, 2017

alan-agius4 commented Dec 17, 2017

dherges commented Dec 19, 2017

dherges commented Dec 19, 2017

dherges commented Dec 19, 2017

alan-agius4 commented Dec 19, 2017

github-actions bot commented Jun 19, 2020

		@@ -0,0 +1,2 @@
		Sample library: external and embeded dependencies
		=====================================

feat: dynamic rollup configuration for esm flattening #395

feat: dynamic rollup configuration for esm flattening #395

Conversation

alan-agius4 commented Dec 11, 2017 • edited by dherges Loading

I'm submitting a...

Checklist

Description

Does this PR introduce a breaking change?

Choose a reason for hiding this comment

alan-agius4 commented Dec 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dherges commented Dec 11, 2017

dherges commented Dec 11, 2017

alan-agius4 commented Dec 11, 2017

alan-agius4 commented Dec 12, 2017 • edited Loading

dherges commented Dec 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dherges commented Dec 17, 2017

alan-agius4 commented Dec 17, 2017

dherges commented Dec 19, 2017

dherges commented Dec 19, 2017

dherges commented Dec 19, 2017

alan-agius4 commented Dec 19, 2017

github-actions bot commented Jun 19, 2020

alan-agius4 commented Dec 11, 2017 •

edited by dherges

Loading

alan-agius4 commented Dec 12, 2017 •

edited

Loading