-
Notifications
You must be signed in to change notification settings - Fork 107
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
Prometheus integration breaks React App #361
Comments
Thanks for that issue @tavogel, I am also experiencing this issue |
@tavogel thanks for report the issue. This has relation with semver-major PR #350. The Jquery example was updated to use opossum 3.0.0 I think that issues with browser will be fixed after #350 if a solution does not come before for the particular case of react usage. |
@tavogel @alexstojda The react example on opossum-examples is working with opossum 3.0.0 By using this way https://github.com/nodeshift-starters/opossum-examples/blob/master/react/client/src/App.js#L2 Please let met know if it work for you |
In my use, I have also added this to my externals: {
'prom-client': 'prom-client'
} This will ensure that webpack ignores the |
Hello again Nodeshift team!
I'd eagerly like to set your Opossum loose on my code. So far so good in Node.js land, but I am experiencing issues on the Browser side, particularly with sites generated using Create React App.
I noticed you have the following example here:
https://github.com/nodeshift-starters/opossum-examples/tree/master/react
This example uses 1.9, which does work. I am able to replicate this issue in your example code with both 2.3 and 3.0 releases, since the addition of Prometheus.
When trying to build or run the library I get the following error:
Opossum has an optional dependency on prom-client so it can export Prometheus metrics. Prometheus requires
cluster
, which is only available server-side:https://nodejs.org/api/cluster.html
This dependency gets required depending on an environment variable WEB not being set:
https://github.com/nodeshift/opossum/blob/master/lib/circuit.js#L8-L10
Ideally we would just set this environment variable at build time, or on startup. But React App does not allow this:
facebook/create-react-app#6223
I also tried modifying the code to use
process.env.REACT_APP_OPOSSUM
and passing that inREACT_APP_OPOSSUM=true npm run build
, but no luck... it might not accept environment variables at this stage, at all. Removing the import line seems to make it work again.Not sure if there is another way to decouple the Prometheus concern from the Circuit Breaker, e.g. by using a wrapper object or something. Might also help to keep bundle sizes down... but that would definitely change your API quite a bit.
Any ideas?
Node.js Version: 10
Operating System: Mac OS
Steps to Produce Error:
git clone git@github.com:nodeshift-starters/opossum-examples.git cd opossum-examples/react/client npm install opossum@^3.0.0 npm run build
The text was updated successfully, but these errors were encountered: