fix(pubsub): fix ESM compatibility issue

[kyle-seongwoo-jun]

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.

[kyle-seongwoo-jun]

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!

[joon-won]

Hey @kyle-seongwoo-jun, thanks for working on a fix for ESM and suggesting alternatives! After assessing the situation, I'm more inclined to go with replacing paho-mqtt with MQTT.js, as paho-mqtt is no longer been maintained. Now I'll go through the steps to address the issues with the suggested changes. Based on what you prefer, you can either open a PR with your feat/pubsub-mqtt branch to main and grant me permission to edit/merge into the branch, or I can work on merging it on behalf of you into main. If you can help us verify the fix once the change goes live, We'd appreciate it.

[kyle-seongwoo-jun]

Hi @joon-won, I just force-pushed to the feat/pubsub-mqtt branch on this PR and checked “Allow edits and access to secrets by maintainers” on GitHub.

If you can help us verify the fix once the change goes live, We’d appreciate it.

Of course! I’d be happy to help anytime.

Thanks! 🙌

[joon-won]

Thanks @kyle-seongwoo-jun 👍 The team will now go through review process + add few more things with the PR.

[kyle-seongwoo-jun]

After checking the error from tsc-compliance-test, I found that the current mqtt is using syntax introduced in TypeScript 4.5 (type modifier on individual named imports), which causes the build to fail on TypeScript 4.2.

I confirmed that downgrading the mqtt version to 5.7.3 allows the build to succeed without any issues. Should we downgrade the mqtt version?

[joon-won]

Thanks for exploring the options @kyle-seongwoo-jun, could also check v5.7.3 works fine. The tsc-compliance test exists as a safety net to provide backward compatibility for existing customers. Let me discuss with my team if we have any feature or bugfix added in between 5.8.0..5.10.4 we consider important.

[AllanZhengYP]

I noticed this change is introducing MqttClient interface to the public newClient method. However the MqttClient is not really compatible with PahoClient, for example onMessageArrived is missed from MqttClient. This could be a breaking change.

We should build an adapter class to map the method to the interfaces we have currently as PahoClient. This would solve the TSC compability test failure as well because the public interfaces in index.d.ts would not have reference to the types exported from 3rd party dependency mqtt.js.

[kyle-seongwoo-jun]

@AllanZhengYP Thanks for the review! I thought MqttClient was only used internally and didn’t realize it was getting exposed through a public method.

I’ll go ahead and make the changes as you suggested.

[joon-won]

Hey @kyle-seongwoo-jun, we can take on from here if you have limited bandwidth to work on this PR. Let us know!