PubSub already loaded console warning

[adamstankiewicz]

In @edx/frontend-platform, running the tests, there are many console warnings about "PubSub already loaded, using existing version". These console warnings can also be seen in some MFE test suites, as well.

PASS src/initialize.test.js
  ● Console
    console.warn
      PubSub already loaded, using existing version
      18 |  */
      19 |
    > 20 | import PubSub from 'pubsub-js';
         | ^
      21 |
      22 | /**
      23 |  *
      at node_modules/pubsub-js/src/pubsub.js:15:17
      at Object.<anonymous> (node_modules/pubsub-js/src/pubsub.js:35:2)
      at Object.<anonymous> (src/pubSub.js:20:1)
PASS src/analytics/interface.test.js

AC

• Identify what's causing the console warnings about PubSub already being loaded.
• Fix so that there are no more console warnings about PubSub already being loaded.

[ishahroz]

When running the test cases, these console warnings come as a result of the singleton pattern being used in the PubSub package. According to this PR, it gives information on if PubSub is already initialised on the root level, that root instance should be used instead of overriding with an empty new one.

After doing root cause analysis, I could pull some of the following observations:

1. These console warnings only come when running the following test suites in @edx/frontend-platform:

• src/initialize.test.js
• src/react/AuthenticatedPageRoute.test

2. In both these test suites, we find common auth interface being imported src/auth/interface.js, and which indirectly imports and employs PubSub module.

3. After removing PubSub wrapper import (import { publish } from '../pubSub') from src/auth/interface.js, we no longer get to see these console warnings.

4. But removing so, it will create ripple since we are using publish method in some of src/auth/interface.js methods. So, to incorporate the removed lines, I wrote their implementation in src/auth/MockAuthService.js but in turn, we get to see the same console warnings after running the test cases.

Moreover, these console warnings merely provide information to reinforce singleton use of PubSub. Maybe instead of passing this information as console warning, it should be documented or set lower message priority level.

[arbrandes]

(/me wears Community Project Manager hat.) @adamstankiewicz, thoughs on @ishahroz's findings?

[adamstankiewicz]

@ishahroz Thanks for investigating. It'd be great if we could set it as a lower message priority level; however, we don't control the code that's generating the console warning. Documentation won't really help reduce the noise in the test output in consuming applications.

[clarification] It's not just the tests in @edx/frontend-platform itself that throw the warning. We are seeing the "PubSub already loaded" console warning when running tests for components that consume @edx/frontend-platform in separate micro-frontend repositories. Generally, we see one warning per test file that consumes @edx/frontend-platform, which adds a lot of unnecessary noise to the test output and sometimes makes it seem like tests are failing when they aren't (e.g., in frontend-app-admin-portal, we have over 200 test files and see this console warning for nearly all of them).

For what it's worth, I noticed there's an existing PR up for pubsub-js around disabling the console.warn. I added notes about our issue as a comment here, with links to @edx/frontend-platform's source and additional example warning stack traces.

/cc @arbrandes

[arbrandes]

I'm marking this as "Blocked" in the Frontend Dev board. In particular, on the upstream merge of the PR @adamstankiewicz pointed out.

[arbrandes]

Upstream commented on the PR:

mroderick/PubSubJS#220 (comment)

@adamstankiewicz, @ishahroz, what are your takes?

[arbrandes]

@adamstankiewicz, @ishahroz: ping! See above comment: looks like upstream reacted.

[adamstankiewicz]

@arbrandes @ishahroz I've responded to the upstream comment, and agree that utilizing a PUBSUBJS_DISABLE_EXISTING_VERSION_WARNING type of optional environment variable makes the most sense for us.

We could set this new environment variable to "true" for all MFEs via @edx/frontend-build's setupTest.js file.

I think we have two options here:

• Make that environment variable upstream and upgrade. This option has my two cents given it seems other consumers of the PubSub library also want to make that console.warn optional and that the maintainer seems willing to accept a contribution for this.
• Patch pubsub-js using the approach in patch-package to remove the console.warn from pubsub-js altogether for our MFEs. Doing so would mean we don't need to wait on an upstream contribution but it does mean we'd be taking on the responsibility of maintaining a (albeit) small patch that might need to be re-implemented when pubsub-js is upgraded.

[arbrandes]

Thanks for taking the time to engage with upstream, @adamstankiewicz! I very much prefer the upstream contribution approach, but I'm not sure @ishahroz would be available to take on the work (includes the back-and-forth until it is merged, etc).

If not, we could leave an issue for the upstreaming open for somebody in our community to take on, and go the patch-package route for a short-term, temporary fix.

[adamstankiewicz]

FWIW, I opened a PR with the environment variable fix upstream here. I'll take on any back-and-forth until it gets merged.

The next steps here would be to:

• Upgrade @edx/frontend-platform to use the new/latest version of pubsub-js with the environment variable fix
• Set process.env.PUBSUBJS_DISABLE_EXISTING_VERSION_WARNING = true in @edx/frontend-build's setupTest.js file.

[arbrandes]

Upstream's been silent on the PR. Pinged them.