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(pubsub): fix ESM compatibility issue #14014

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kyle-seongwoo-jun
Copy link

Description of changes

This PR resolves the issue where using paho-mqtt.js in an ESM project caused the error: Paho.Client is not a constructor. The root cause was that paho-mqtt.js is distributed as a UMD module, which is incompatible with native ESM imports.

To address this, the build process has been updated to include the @rollup/plugin-commonjs plugin. This plugin ensures that UMD modules like paho-mqtt.js are correctly transformed into ESM-compatible modules during the ESM build process.

This change allows MqttOverWS to use Paho.Client without any compatibility issues.

Issue #, if available

Description of how you validated changes

  1. Imported the updated package into an ESM project.
  2. Verified that the Paho.Client is not a constructor error no longer occurs during runtime.
  3. Tested the MQTT connection functionality to ensure it works as expected with the MqttOverWS implementation.
  4. Confirmed that the Rollup build process completes without errors and generates correct ESM-compatible output.

Checklist

  • PR description included
  • yarn test passes
  • Unit Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)

Checklist for repo maintainers

  • Verify E2E tests for existing workflows are working as expected or add E2E tests for newly added workflows
  • New source file paths included in this PR have been added to CODEOWNERS, if appropriate

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Added @rollup/plugin-commonjs to handle CommonJS modules in ESM build, ensuring compatibility with paho-mqtt.js.
@cwomack cwomack added PubSub Related to PubSub category external-contributor labels Nov 26, 2024
@kyle-seongwoo-jun
Copy link
Author

kyle-seongwoo-jun commented Nov 28, 2024

Hi AWS Amplify Team(@HuiSF @cwomack),

First, I want to thank you for all the hard work and dedication you’ve put into this project.

Recently, I migrated my project to ESM and encountered this issue. As a workaround, I’ve been using patch-package to modify the dist code in node_modules to make it work.

After reviewing the code blame of the pubsub package, it seems that there have been issues with ESM support since its very first version.

The root cause of this problem lies in the fact that the pubsub package uses paho-mqtt.js, which doesn’t support ESM. Based on this, I’ve considered two potential solutions:

  1. Convert paho-mqtt.js to ESM during the Rollup build process.
    This is the approach I implemented in this PR. It doesn’t affect the existing CJS builds, so it should be safe and side-effect-free, making it a reasonable solution for the widely used AWS Amplify project.

  2. Replace paho-mqtt with MQTT.js.
    Considering that paho-mqtt is effectively no longer actively maintained, switching to the more actively supported MQTT.js would likely be better in the long term. However, this change might introduce unforeseen issues in various browser environments, so it would require extensive testing.
    I’ve also created a version implementing this approach: main...kyle-seongwoo-jun:amplify-js:feat/pubsub-mqtt. In my environment, the MQTT.js version works perfectly well.

I’d love to hear your thoughts and see how these solutions align with your policies or long-term plans. My goal is to help in any way I can. Thank you for your time and consideration!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external-contributor PubSub Related to PubSub category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants