add support for multiple apps using module federation #307

hasanayan · 2021-08-19T20:13:17Z

this is a recreated/clean version of #291

I had to disable a few es-lint rules;

index.js line 82-83: adding webpack-sources as dependency breaks the tests (some of the tests use older version(s) of node than webpack-sources requires as minimum). On the other hand, if the user is using module federation, their webpack version is at least 5 and webpack-sources come with it. So, adding webpack-sources as a dependency is not a must. For the same reason, I did not put the require statement at the top of the file (it might be missing for users using an older version of webpack)
index js line 105; it is what it is, the rule says no-underscore-dangle but the object coming from webpack has it 🤷🏼‍♂️

TL;DR;

the plugin now injects additional SENTRY_RELEASES variable into global scope as an object that holds versions for all apps keyed by [project]@[org]

When ModuleFederationPlugin is used, assuming the project has a single entry in the webpack config, the build outputs a bundle with two separate entrypoints; usually named as 'main' and 'remoteEntry'. Main is used when the app is running in standalone (host) mode and remoteEntry is used when it is consumed as a remote by another app. In our projects we needed to access the version value for all remotes on our host app so that we can create SentryClient per remote with their own respective versions.

With this PR, the code injected by loader is improved to include a second variable SENTRY_RELEASES in addition to SENTRY_RELEASE. This code goes to main module and is not loaded when the app is running as remote. So I also added the part to tap into compilation and inject the variable in remoteEntry as well.

If this is accepted, we can maybe open another PR on @sentry/browser so that the default release is read from SENTRY_RELEASES and retire SENTRY_RELEASE

hasanayan · 2021-08-19T20:17:25Z

@kamilogorek I hope you had a good time in your vacation. I recreated the PR I opened earlier. I had to disable some es-lint rules (on specific lines) so I don't introduce breaking changes (if you think we can break support for earlier versions of node, I can update the PR). could you please run the tests?

kamilogorek

Only one small change. Better safe than sorry.
Thanks for your patience, I got pulled off to other things after coming back.

src/index.js

kamilogorek · 2021-09-06T10:11:06Z

Failing test should be trivial to fix, as its just string based assertion.

reposition eslint comment Co-authored-by: Kamil Ogórek <kamil@sentry.io>

hasanayan · 2021-09-06T13:17:38Z

@kamilogorek thanks. I approved your suggestion. I’m on mobile, if there are other changes required I l’ll have a look in a few days. (now is my vacation time :) )

kamilogorek · 2021-09-06T13:21:35Z

I cannot push to your fork so will have to wait for you. No rush, it won't go anywhere. Enjoy your time off!

hasanayan · 2021-10-08T14:45:14Z

Hi @kamilogorek , I think this time I managed to pass the tests.

Also added 2 more steps in the integration test that tests my change partially. Can't do a full test because, It would need another test project that uses webpack 5 and that does not work on the older versions of Node. If you would like to have tests for that as well, we can discuss. Otherwise, the PR should be complete and pass the tests at the moment.

Could you please run the workflows?

kamilogorek · 2021-10-11T11:02:16Z

@lobsterkatie @AbhiPrasad @iker-barriocanal can I get a second pair of eyes on this? Would prefer this, as its non-trivial change. +1 from me, so feel free to merge it once approved, as all tests are green.

iker-barriocanal

I rather not merging it myself since I don't have context on this webpack plugin, just had a look at the code and can't say whether the bigger picture looks good.

src/sentry.loader.js

iker-barriocanal · 2021-10-12T08:06:05Z

example/test.js

+  ) !== -1
 ) {
  console.log('Saul Goodman, found SENTRY_RELEASE in bundle');
-  process.exit(0);


Why remove this process.exit, and why not add it to the previous check? Should we expect to find all the assignments? If we successfully find one but fail the others, should we exit? If this is the case, should we move the process.exit(0) in the last block to run after it?

there used to be only one step check of existence on a string. Now the test does 3-step check (for existence of 3 different strings). when any of them fails, we call process.exit(1). However, calling process.exit(0) should be done when only the last check passes successfully because; let's say if we put it on success case of the first case, the test will simply finish without continuing on the next steps.

If this is the case, should we move the process.exit(0) in the last block to run after it?

So moving the last process.exit(0) out of the block, so that the logic is the same?

Instead of:

if (!A) exit(1) if (!B) exit(1) if (C) exit(0) if (!C) exit(1)

this instead:

if (!A) exit(1) if (!B) exit(1) if (!C) exit(1) exit(0) <-- and omit this?

Agree with Iker. Updated the test. Last process.exit(0) is redundant anyway, as scripts by default exit with 0

Co-authored-by: iker barriocanal <32816711+iker-barriocanal@users.noreply.github.com>

hasanayan · 2021-10-12T08:22:46Z

@iker-barriocanal I gave quite an extensive explanation on the context (please see my PR description). If that is not enough, we can move this discussion on a live call and I can explain you the problem we are facing (and other people will face when they start using module federation more and more) and the solution I am proposing.

iker-barriocanal · 2021-10-12T08:41:49Z

@hasanayan sorry, I wasn't specific enough. I'm not familiar with this webpack plugin and thus don't want to make the merging decision, which isn't related to your PR at all. Just did a simple review code-wise so I'm definitely not blocking. Updated the comment.

hasanayan · 2021-10-12T08:49:07Z

@iker-barriocanal got it. thanks :)

example/test.js

kamilogorek · 2021-10-12T10:40:50Z

@hasanayan could you verify this change with your current setup before we make any releases? You can use the plugin's master branch with either yarn add https://github.com/getsentry/sentry-webpack-plugin.git#master or using local copy with file:./path/to/plugin.

hasanayan · 2021-10-12T10:41:40Z

@kamilogorek sure! on it now

hasanayan · 2021-10-12T10:53:47Z

@kamilogorek it works fine! thanks for all the effort! 👊🏻

kamilogorek · 2021-10-12T12:34:52Z

Awesome, thanks for verifying! Released as 1.18.0

raine · 2021-10-28T12:21:16Z

Hi @hasanayan,

This PR looks interesting. Currently working on Sentry and federated modules in a project.

In our projects we needed to access the version value for all remotes on our host app so that we can create SentryClient per remote with their own respective versions.

Could you explain why the remotes need the host app's release version if they have their own respective versions? I assume term version is always interchangeable with same value that appears in Sentry.init() object for instance. Thanks in advance!

hasanayan · 2021-10-28T15:52:19Z

Hi @hasanayan,

This PR looks interesting. Currently working on Sentry and federated modules in a project.

In our projects we needed to access the version value for all remotes on our host app so that we can create SentryClient per remote with their own respective versions.

Could you explain why the remotes need the host app's release version if they have their own respective versions? I assume term version is always interchangeable with same value that appears in Sentry.init() object for instance. Thanks in advance!

Hi @raine , accessing versions of remotes from the host app is requirement of our own implementation. We have some cases that require this. For example; if a remote is not accessible at any moment, we want to to be able to report this to the remote's Sentry project with its own respective version.

The purpose of this PR was to inject sentry versions of each app (host and all the remotes) into global scope without then colliding with each other. Where you want to access that is up to you. Before this PR, if you would call Sentry.init from any app (host or remote) it would use SENTRY_VERSION variable that is in the global scope and the value would be the hosts version. Because, previously. the variable would be injected only to main (standalone) entrypoint and not the remoteEntry.

dfdeagle47 · 2021-12-30T11:18:44Z

@hasanayan I'm investigating a broken build due to ES6 syntax in our app bundle, so forgive me if my assumption is incorrect, but won't this line represent a breaking change:

e621cea#diff-e4d441b957e997f8330cdbc7252040be3195c8af431297cf012aa70c00f66c19R5

The injected Sentry release string contains the const keyword:

let sentryRelease = `const _global = (typeof window !== 'undefined' ? window : typeof global !== 'undefined' ? global : typeof self !== 'undefined' ? self : {}); _global.SENTRY_RELEASE={id:"${version}"};`;

but the previous code did not contain the const keyword in the Sentry release string (if I'm reading the changes right):

const sentryRelease = `(typeof window !== 'undefined' ? window : typeof global !== 'undefined' ? global : typeof self !== 'undefined' ? self : {}).SENTRY_RELEASE={id:"${version}"};`;

So if the consumer of this lib doesn't transpile external libs, it could break on browsers which don't support ES6+ syntax.

Happy to provide a PR (and/or create an Issue) if you think the const should be changed to var for compatibility purposes.

kamilogorek · 2022-01-11T11:44:03Z

You are correct @dfdeagle47, thanks for catching this. Updated in #338

In #307, a dependency on `webpack-sources` was added to the code, but not to `package.json`. From that PR description: > Adding webpack-sources as dependency breaks the tests (some of the tests use older version(s) of node than webpack-sources requires as minimum). On the other hand, if the user is using module federation, their webpack version is at least 5 and webpack-sources come with it. So, adding webpack-sources as a dependency is not a must. For the same reason, I did not put the require statement at the top of the file (it might be missing for users using an older version of webpack) Though this logic is generally sound, it (specifically the "their webpack version is at least 5 and webpack-sources come with it" part) doesn't account for cases in which webpack is being consumed from a single bundle. In those cases, the _code_ from `webpack-sources` exists in the user's dev environment, but the actual `webpack-sources` _package_ doesn't. (This is true, for example, in nextjs, which pre-compiles many of its dependencies, including webpack[1], into minified bundles.) This can lead to errors when trying to use module federation alongside our nextjs SDK. To fix this, `webpack-sources` has been added as a first-class runtime dependency. Because that guarantees that it will be present and `require`-able, its import has been moved out of a mid-file `try-catch` to instead be a legit import at the top of the file in which it's used. [1] https://github.com/vercel/next.js/blob/c2f48ea86d448c8be1982b46f184b1d2f2d6cd50/packages/next/compiled/webpack/bundle5.js

add support for multiple apps using module federetion

529cf3e

hasanayan mentioned this pull request Aug 19, 2021

Add Module Federation Support #291

Closed

kamilogorek suggested changes Sep 6, 2021

View reviewed changes

src/index.js Outdated Show resolved Hide resolved

Update src/index.js

6ff7cef

reposition eslint comment Co-authored-by: Kamil Ogórek <kamil@sentry.io>

fixed failing integration test

3fb1970

iker-barriocanal reviewed Oct 12, 2021

View reviewed changes

Update src/sentry.loader.js

576615f

Co-authored-by: iker barriocanal <32816711+iker-barriocanal@users.noreply.github.com>

kamilogorek reviewed Oct 12, 2021

View reviewed changes

example/test.js Show resolved Hide resolved

Update example/test.js

fddc3de

kamilogorek self-requested a review October 12, 2021 10:38

kamilogorek approved these changes Oct 12, 2021

View reviewed changes

kamilogorek merged commit e621cea into getsentry:master Oct 12, 2021

kamilogorek mentioned this pull request Jan 11, 2022

ref: Use var instead of const for module loader for ES5-compat #338

Merged

lobsterkatie mentioned this pull request Oct 6, 2022

fix(deps): Add webpack-sources dependency #397

Merged

Lms24 mentioned this pull request Nov 9, 2022

feat(core): Add SENTRY_RELEASES variable during release injection getsentry/sentry-javascript-bundler-plugins#102

Merged

Uh oh!

add support for multiple apps using module federation #307

add support for multiple apps using module federation #307

Uh oh!

Conversation

hasanayan commented Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hasanayan commented Aug 19, 2021

Uh oh!

kamilogorek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kamilogorek commented Sep 6, 2021

Uh oh!

hasanayan commented Sep 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kamilogorek commented Sep 6, 2021

Uh oh!

hasanayan commented Oct 8, 2021

Uh oh!

kamilogorek commented Oct 11, 2021

Uh oh!

iker-barriocanal left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iker-barriocanal Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

hasanayan Oct 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iker-barriocanal Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

kamilogorek Oct 12, 2021

Choose a reason for hiding this comment

Uh oh!

hasanayan commented Oct 12, 2021

Uh oh!

iker-barriocanal commented Oct 12, 2021

Uh oh!

hasanayan commented Oct 12, 2021

Uh oh!

Uh oh!

kamilogorek commented Oct 12, 2021

Uh oh!

hasanayan commented Oct 12, 2021

Uh oh!

hasanayan commented Oct 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kamilogorek commented Oct 12, 2021

Uh oh!

raine commented Oct 28, 2021

Uh oh!

hasanayan commented Oct 28, 2021

Uh oh!

dfdeagle47 commented Dec 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kamilogorek commented Jan 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hasanayan commented Aug 19, 2021 •

edited

Loading

hasanayan commented Sep 6, 2021 •

edited

Loading

iker-barriocanal left a comment •

edited

Loading

hasanayan Oct 12, 2021 •

edited

Loading

hasanayan commented Oct 12, 2021 •

edited

Loading

dfdeagle47 commented Dec 30, 2021 •

edited

Loading