braze-inc / braze-web-sdk

Public repo for the Braze Web SDK
https://www.braze.com
Other
71 stars 25 forks source link

[Bug]: Circular dependencies causing error with Vite #123

Closed pioug closed 2 years ago

pioug commented 2 years ago

Braze Web SDK Version

4.1.0

Integration Method

NPM

Browser

Chrome

Steps To Reproduce

You can try with https://github.com/pioug/braze-circular-dependencies

You can see the warnings in Rollup logs:

> npm run rollup

> rollup
> rollup -c

main.js → build...
(!) Circular dependencies
node_modules/@braze/web-sdk/src/common/event-logger.js -> node_modules/@braze/web-sdk/src/managers/braze-instance.js -> node_modules/@braze/web-sdk/src/managers/network-manager.js -> node_modules/@braze/web-sdk/src/common/event-logger.js
node_modules/@braze/web-sdk/src/common/event-logger.js -> node_modules/@braze/web-sdk/src/managers/braze-instance.js -> node_modules/@braze/web-sdk/src/request-controller.js -> node_modules/@braze/web-sdk/src/common/event-logger.js
node_modules/@braze/web-sdk/src/managers/braze-instance.js -> node_modules/@braze/web-sdk/src/triggers/triggers-provider-factory.js -> node_modules/@braze/web-sdk/src/InAppMessage/in-app-message-manager-factory.js -> node_modules/@braze/web-sdk/src/InAppMessage/in-app-message-manager.js -> node_modules/@braze/web-sdk/src/managers/braze-instance.js
...and 48 more
created build in 1s

The package can't be imported when using Vite dev server:

> npm run vite

In the browser console:

full-screen-message.js:1 Uncaught TypeError: Class extends value undefined is not a constructor or null
    at full-screen-message.js:1:39

Expected Behavior

No circular dependencies warnings or errors when importing the SDK

Actual Incorrect Behavior

Right now, the SDK can't be imported by the Vite dev server. Though the error message is a bit obscure, I think it is due to the circular dependencies.

Verbose Logs

No response

Additional Information

The improvements from v3 to v4 sound really great! I am looking forward to upgrade the SDK for BandLab 🙌

wesleyorbin commented 2 years ago

Hi @pioug. I'm sorry you're having this issue. I was able to fix the issue locally by reconfiguring some imports in the SDK. We can release the fix sometime next week and I'll file a ticket internally to holistically solve the circular dependencies.

wesleyorbin commented 2 years ago

Hi @pioug! Apologies for the delay, but we just released v4.0.5 which should fix the build error. I'm going to close out the issue, but feel free to re-open or file another issue should you encounter other issues.

pioug commented 2 years ago

@wesleyorbin I updated to v4.0.5 and I see this error now when using Vite:

Uncaught TypeError: Class extends value undefined is not a constructor or null
    at html-message.js:1:47

I updated https://github.com/pioug/braze-circular-dependencies if you want to try

wesleyorbin commented 2 years ago

@pioug Ah, I changed./main.js to ./build/main.js in the index.html and that's probably why I no longer saw the error before. It appears that when pre-bundling the SDK in the dev server, Vite defines InAppMessage below some of its subclasses. I was able to solve this particular issue locally, but I worry that other similar errors may surface until we holistically resolve the circular dependencies.

Is there a way to exclude our SDK from Vite's pre-bundling in the meantime? Alternatively you could import the SDK in the HTML temporarily, as it is compatible with native ES Modules.

<script type="module">
  import * as braze from "./node_modules/@braze/web-sdk/src/index.js"
  console.log(braze);
</script>
yungblud commented 2 years ago

Same issue here, I am using vite with React. I think it would be resolved by resolving circular dependency issue. We can check circular dependency issues by cloning this repo (https://github.com/pioug/braze-circular-dependencies) and run yarn rollup.

I think if all circular dependency issues were resolved, we can use braze web sdk v4 with vite too.

marcanft commented 2 years ago

Hi!

Do we have any news about this issue?

davidbielik commented 2 years ago

Hi @marcanft this is something we're planning to investigate in the next few weeks.

Are you able to use any of Vite's advanced configuration to exclude @braze/web-sdk as previously suggested?

I found this article or this Stackoverflow answer which may help.

marcanft commented 2 years ago

Thanks @davidbielik!

optimizeDeps: {
    exclude: ['@braze/web-sdk']
},

This worked for me

davidbielik commented 2 years ago

Glad to hear that workaround is working for you @marcanft. We have work planned in the future to address these, so I'll close this out while we track this internally.

onlywei commented 6 months ago

I don't understand why setting @braze/web-sdk as an external is desirable. Doesn't that just tell Rollup to exclude @braze/web-sdk from the bundle? That would mean nothing within @braze/web-sdk can ever make it to the browser, which would result in runtime errors. Is that the correct understanding?

Also, I'm experiencing this error on version 5.1.1 of @braze/web-sdk...

wesleyorbin commented 6 months ago

Hi @onlywei. The above solution only excludes the web SDK from Vite's dependency optimization in the dev server. When you run a production build, it will still be included by Rollup.