apancutt / react-mixpanel-browser

MIT License
28 stars 6 forks source link

Remove process.env dependency #17

Closed gkedzierski closed 1 year ago

gkedzierski commented 2 years ago

Currently this package depends on the process.env not being undefined at build time, which fails on some build systems. (like Vite) This pull request removes the logic that grabs process.env.REACT_APP_MIXPANEL_TOKEN and assumes it will always be provided as a MixpanelProvider prop.

As this would be a breaking change for anyone that uses this package and doesn't provide token explicitly, I bumped the major version of the package.

I added a basic .prettierrc config, as it looks like the package was using single quote characters, and most modern editors default to double quotes.

I understand the change may be a little controversial if you're not planning on supporting any other build systems, in which case feel free to close this PR. (and we'll continue using the package based on our fork)

apancutt commented 2 years ago

This change seems redundant; there is no change in behaviour when the envvar is unavailable and instead this change only introduces a regression for users that depend on current functionality. Have I missed something?

Site note: It looks like Vite supports environment variables so this might be an option for you.

gkedzierski commented 2 years ago

Vite doesn't know about process and therefore doesn't do static replacement, (like it does for import.meta.env.* which is Vite's equivalent) so when the output bundle contains process.env.REACT_APP_MIXPANEL_TOKEN, which then gets executed in the browser environment and throws Uncaught ReferenceError: process is not defined exception.

I found a possible workaround by using define property in the Vite config, like:

define: {
  'process.env.NODE_ENV': JSON.stringify(process.env.NODE_ENV),
},

This seems to work so if you're not keen on merging the change, lets close the PR and it'll hopefully get indexed for anyone who encounters the issue in the future!

apancutt commented 2 years ago

Are you building your project using react-scripts? That should take care of removing references to process.env so that the output is browser-safe.

gkedzierski commented 2 years ago

Vite is completely separate from react-scripts.

billsbooth commented 1 year ago

Having a similar issue as well while using Next.js. Would be ideal to have it be passed from the top and provide the optionality for end user.

apancutt commented 1 year ago

Fixed in 4.0.0