janhommes / o.js

o.js - client side oData lib.
https://janhommes.github.io/o.js/example/
MIT License
238 stars 57 forks source link

Node incompatibility #120

Closed thomasmarr closed 3 years ago

thomasmarr commented 3 years ago

I recently had to port some client side code to run on node, and the o.js dependency would not run. There were a couple of places where it threw a Reference Error. ("Headers is not defined" and "Request is not defined").

The readme has a snippet for node - does o.js generally run on node? If not, you can see the fixes I made to get it working for my needs here: https://github.com/thomasmarr/o.js/commit/b176183ddd78e558d99847fc3c9fb3f9dc36d3d3

@janhommes let me know if this has any value, and you would like me to submit a PR.

janhommes commented 3 years ago

Just tried the example here and it's working fine for me: https://github.com/janhommes/o.js/blob/master/example/node.js

Yes, o.js was intended to work in the browser and in node. But maybe there is something wrong! Can you share the node version you are using? Or any example?

Your PR would make it work in node but I guess it might break it in the browser.

thomasmarr commented 3 years ago

The project is running node v12.22.1

Yes, if you wanted me to submit a PR I would probably try something like if (typeof window !== 'undefined') ... for the Headers import.

Or potentially avoid Headers altogether - I haven't really got my head around the need for it. Similarly for using Request - I ended up removing that line. If there's a strong reason to use Request and Headers in those places I am very open to hearing and understanding why.

I'd really like to get a fix into this package if possible, to save needing a separate fork just for that minor issue.

janhommes commented 3 years ago

When I remember correctly I used it to pleasure the typing of Typescript. I can not reproduce the issue. Can you show me an example when it is failing?

janhommes commented 3 years ago

Can't reproduce.

thomasmarr commented 3 years ago

@janhommes apologies I was snowed under and just getting around to this now. Snippet below which produces the error. I've omitted the types, but it's not TypeScript which is complaining so that shouldn't matter, it's a runtime error with node.

FWIW the project is on node v12.22.1

import { o } from 'odata'

const query =  async ({ resource, query }) {
   const config = {
      headers: {
        'api-key': 'a_valid_api_key',
        Authorization: `Bearer ${a_valid_token}`,
        'Content-Type': 'application/json',
      },
    }
    const data = await o(apiUrl, config).get(resource).query(query)
    console.log(data)
  }
}

query({resource:'a_valid_api_resource', query:{$top: 1}})

Stacktrace:

System.Private.CoreLib: Exception while executing function: function_name. System.Private.CoreLib: Result: Failure
Exception: ReferenceError: Headers is not defined
Stack: ReferenceError: Headers is not defined
 at Object.o (repo/node_modules/odata/dist/cjs/o.js:613:26)

If I resolve that error, then I get a similar Request is not defined error at src/ORequest.ts:15

janhommes commented 3 years ago

Hey, thanks for giving that example. I created the following test.ts file in an empty repository:

import { o } from "odata";

const apiUrl = "https://services.odata.org/V4/TripPinServiceRW/";

const query = async ({ resource, query }) => {
  const config = {
    headers: {
      "api-key": "a_valid_api_key",
      // Authorization: `Bearer ${a_valid_token}`,
      "Content-Type": "application/json",
    },
  };
  const data = await o(apiUrl, config).get(resource).query(query);
  console.log(data);
};

query({ resource: "People?$top=2", query: { $top: 1 } });

And then compile it with: npx tsc test.ts --lib ES2015,dom

And running it simply with node: node test.js

it's working fine. Did you maybe not add the dom typescript lib? Do you assume it should not be needed for this?

thomasmarr commented 3 years ago

Sorry for the delay, this is the first time I've had a chance to dig into it since last time I commented.

Adding --lib ES2015,dom doesn't help. The issue is happening at runtime, in the compiled JS. I think these libs just specify which type definitions you want to pass to the compiler.

With a lot of digging I have discovered the root cause. This library has a polyfill from cross-fetch which is included conditionally if(!('fetch' in env) && .......

The cross-fetch package itself also has a conditional if (!global.fetch) before applying the polyfill.

Somehow, in the project environment global.fetch is defined, but Headers and Request are not. So both o.js and cross-fetch see that fetch is defined so the polyfill doesn't get applied.

I have not yet been able to identify why this project has fetch defined globally but not Headers or Request.

thomasmarr commented 3 years ago

Inspecting the global object in debugger I found that the fetch definition originated in node-fetch. The project itself does not use node-fetch directly but some of its dependencies do.

Eventually I managed to figure out which dependency was causing it. Thankfully, they have changed that behaviour in a more recent version.

After upgrading that dependency, fetch is no longer defined on the global object, and the polyfill starts working again.

It was a very difficult problem to debug, so I'm leaving these details in case anyone else comes up against it. If one of your dependencies adds fetch to the global object without adding Headers or Request, then the cross-fetch polyfill will not apply (currently version 3.1.4), and odata will trip up at Headers is not defined and Request is not defined.