-
Notifications
You must be signed in to change notification settings - Fork 171
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
Node standard "os" error #465
Comments
exactly, I remove those lines and it works |
Which version of node is producing this error? Can you paste the full traceback if there's more? I can't tell what's actually throwing the error here. |
It is simple, I am running this on React, no Node standard library runs on it but base on the package description it shouldn't happen |
Oh, this is running in a browser environment? I wonder if we need to wrap the import-level execution in a check for that? |
doesn't look bad. Maybe this is helpful https://github.com/flexdinesh/browser-or-node#readme, without any extra package this can be used: |
@santiagoalmeidabolannos just for additional clarification, do you see this error in the browser console at runtime or in a warning at build-time? Just curious for future reference, and also to understand whether this breaks the build or causes errors or something else. |
@jdb8 I am using React Native with Expo to be exact, I get the error when Expo is building the bundle. I guess the same will happen during the build process (prod or dev server) on any modern JavaScript framework. |
Any chance any of you can come up with a fix for this? |
I am not sure which of your modules are currently using Node standard library modules |
Ping @jdb8 |
So if I understand correctly, this issue is happening because the code is being imported outside of a node environment: and my change introduced an import-time side effect which assumes the environment is node. @jcchavezs are there any pre-existing tests for this case? I wrote my code with the assumption that it would only be called in node, but if this package supports other environments, it would be worth setting up an integration test for this + any other import issues. Please correct me if I'm missing any context here. |
This is the first sentence of the README file 😊 |
@jdb8 will this work as an example? https://github.com/openzipkin/zipkin-js-example/tree/master/react-native-example
|
That's great but I still can't use the latest version of your package. A PR is needed to make the usage/import if node standard modules optional depending on the environment. @jdb8 can you help with that? |
@jcchavezs @santiagoalmeidabolannos I don't have a ton of time at the moment to work on a fix for this, but it sounds like the problem is well-understood (needing to wrap this in an environment check). My question around the test was mainly to work out if there's an existing CI setup to run the code in a browser or react-native environment, since presumably this would be caught by that. I'm assuming such a test suite doesn't exist otherwise my change would have failed CI. |
Android Bundling failed 8902ms (E:\programm files\GITHUB\React_native-firstProject\node_modules\expo\AppEntry.js) |
I am getting this
The package at "node_modules\zipkin\lib\index.js" attempted to import the Node standard library module "os".
it should not be happening.I add this into my
tsconfig.json
"paths": { ... "node-fetch": [ "node_modules/empty-module/index.js" ], "os": [ "node_modules/empty-module/index.js" ] },
The text was updated successfully, but these errors were encountered: