openzipkin / zipkin-js

Zipkin instrumentation for Node.js and browsers
Apache License 2.0
564 stars 171 forks source link

Node standard "os" error #465

Open santiagoalmeidabolannos opened 4 years ago

santiagoalmeidabolannos commented 4 years ago

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" ] },

jcchavezs commented 4 years ago

Could be related to https://github.com/openzipkin/zipkin-js/commit/b3b5a0ca7466f76e9139f2f9409d5af4f42ea3f7? cc @jdb8

santiagoalmeidabolannos commented 4 years ago

exactly, I remove those lines and it works

jdb8 commented 4 years ago

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.

santiagoalmeidabolannos commented 4 years ago

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

jdb8 commented 4 years ago

Oh, this is running in a browser environment? I wonder if we need to wrap the import-level execution in a check for that?

santiagoalmeidabolannos commented 4 years ago

doesn't look bad. Maybe this is helpful https://github.com/flexdinesh/browser-or-node#readme, without any extra package this can be used: var isBrowser=new Function("try {return this===window;}catch(e){ return false;}"); or var isNode=new Function("try {return this===global;}catch(e){return false;}");

jdb8 commented 4 years ago

@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.

santiagoalmeidabolannos commented 4 years ago

@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.

jcchavezs commented 4 years ago

Any chance any of you can come up with a fix for this?

santiagoalmeidabolannos commented 4 years ago

I am not sure which of your modules are currently using Node standard library modules

jcchavezs commented 4 years ago

Ping @jdb8

jdb8 commented 4 years ago

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.

santiagoalmeidabolannos commented 4 years ago

This is the first sentence of the README file 😊 This is a set of libraries for instrumenting Node.js and browser applications. The zipkin library can be run in both Node.js and the browser.

jcchavezs commented 4 years ago

@jdb8 will this work as an example? https://github.com/openzipkin/zipkin-js-example/tree/master/react-native-example

santiagoalmeidabolannos commented 4 years ago

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?

jdb8 commented 4 years ago

@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.

Ruwiseturtle commented 5 months ago

Android Bundling failed 8902ms (E:\programm files\GITHUB\React_native-firstProject\node_modules\expo\AppEntry.js) The package at "node_modules\@expo\metro-config\build\ExpoMetroConfig.js" attempted to import the Node standard library module "os". It failed because the native React runtime does not include the Node standard library. Learn more: https://docs.expo.dev/workflow/using-libraries/#using-third-party-libraries уже неделю с этим разбираюсь. Что это за ошибка? как ее исправить?