Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(plugin-angular): update bundles and package entrypoints to support Ivy #994

Merged
merged 3 commits into from
Aug 20, 2020

Conversation

djskinner
Copy link
Contributor

@djskinner djskinner commented Aug 13, 2020

Fixes #735

Before

We produced two bundles:

  • /dist/es5/ - which contained ES5 + ES modules (import but no class)
  • /dist/esm/ - which contained ES2015 (import and class)

The various entry points we configured as follows:

  "main": "dist/es5/index.js",
  "browser": "dist/es5/index.js",
  "module": "dist/esm/index.js",
  "es2015": "dist/esm/index.js",
  "esm5": "dist/es5/index.js",

The problem for the reproducible example supplied (with "enableIvy": true) was:

Compiling @bugsnag/plugin-angular : module as esm5

[...]

ERROR in Attempted to get members of a non-class: "class BugsnagErrorHandler extends ErrorHandler {

It's worth noting that this problem seems to have gone away in more recent version. In an example which uses more recent versions (using newer versions of angular, again with Ivy enabled):

Compiling @bugsnag/plugin-angular : es2015 as esm2015

And the build is successful.

Problem

In the supplied example (which was using mostly angular ~8.2.14 dependencies plus @angular/cli ~8.3.25) webpack is using the module target and expecting esm5. However that bundle (/dist/esm/) was actually compiled to ESM2015. This would seem to be correct according to this document, which gives an example: "module": "./fesm2015/core.js". However, when looking at how the packages are actually implemented, we see something different. Both @angular/core@8.2.14 and @angular/core@9.1.12 specify "module": "./fesm5/core.js", rather than the ./fesm2015/core.js expected from the documented spec.

It's worth noting also that neither of these package versions specify a browser field.

After

If, rather than going with what the document says we replicate that @angular/core does, the problem is resolved for the failed Ivy test case (8.2.14) and continues to work in the 9.1.12 example and the existing E2E test.

The changes are:

  • Rename dist/es5 to dist/esm5 to more accurately represent what it is
  • Rename dist/esm to dist/esm2015 to more accurately represent what it is
  • Point module and esm5 to dist/esm5 instead of dist/esm2015 to satisfy the 8.2.14 test case

Note:

In the original commit I had created a third es5 proper bundle and pointed browser and main to that. This is what caused the E2E test case to fail. I did this because I assumed browser should always point to code that is ready to be run in any supported browser (so we'd have to assume ES5) but it seems at least for angular esm5 is expected.

With an ES5 bundle we got a build error:

WARNING in ./node_modules/@bugsnag/plugin-angular/dist/es5/index.js
25:24-31 Critical dependency: require function is used in a way in which dependencies cannot be statically extracted

and a runtime error:

Uncaught Error: Cannot find module "."

It's worth noting that removing the browser field also fixed this error.

Testing process

  • Build and start the app.

  • Check no build errors in console or runtime errors in browser

  • Clear dist, node_modules and re-install with npm i

  • In tsconfig.json add "enableIvy": true to angularCompilerOptions

  • Build and start the app.

  • Check no build errors in console or runtime errors in browser

  • 8.2.14 error reproduction

  • 9.1.12 example project (Start from https://angular.io/tutorial/toh-pt0 and install Bugsnag as per our docs)

I suggest further testing is done on the various versions and varieties of angular apps and configurations we support.

@bugsnagbot
Copy link
Collaborator

bugsnagbot commented Aug 13, 2020

@bugsnag/browser bundle size diff

Minified Minfied + Gzipped
Before 39.62 kB 12.23 kB
After 39.62 kB 12.23 kB
± No change No change

Generated by 🚫 dangerJS against 8890726

@bengourley bengourley requested a review from a team August 20, 2020 10:38
Copy link
Contributor

@bengourley bengourley left a comment

Choose a reason for hiding this comment

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

Looks good to me. Tested with various versions of angular, modifying:

  • enableIvy=true|false
  • target=es3|es5|es2015

And including build flags that have scuppered things in the past: --prod --aot.

Tagged @bugsnag/platforms-test-automation for a cursory glance if you think additional testing is necessary or if you just want to wave it through.

@bengourley bengourley merged commit 51b79b1 into next Aug 20, 2020
@bengourley bengourley deleted the update-plugin-angular-bundles branch August 20, 2020 13:33
@imjoehaines imjoehaines mentioned this pull request Aug 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Angular build failes with Ivy renderer due to error in bugsnag
3 participants