Skip to content
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

Update fetchAdapter.js to use globalThis instead of global. #428

Closed
wants to merge 1 commit into from
Closed

Update fetchAdapter.js to use globalThis instead of global. #428

wants to merge 1 commit into from

Conversation

twobitunicorn
Copy link

Notes

Sorry. no Jira ticket.

How to test

I had a situation where I needed faunadb to run in an SSR environment on top of Vite. global was not found so I changed it to globalThis which handled the clientside compile for me.

Screenshots

Copy link

@entioentio entioentio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me, I run into this as well

@fireridlle
Copy link
Contributor

globalThis is not supported by IE and some old versions of browsers as well as nodejs lower 12.0.0 that we still support.

@twobitunicorn
Copy link
Author

twobitunicorn commented Mar 19, 2021

@fireridlle as far as my limited knowledge goes; this currently doesn't work in any browser. At least that is what my SSR setup is telling me. Do you have another recommendation to enable the driver to work in browsers?

@fireridlle
Copy link
Contributor

I think it doesn't work at SSR for some reason. I have just tested on react create app and vuex and wasn't able to reproduce.
can you share the framework you are using? If you have any custom config for webpack (or any other bundle tool), having it also would help to debug

@twobitunicorn
Copy link
Author

@fireridlle Using an empty project I am unable to reproduce it. I was using Svelte/Sapper. I moved over to using it again in my project and it appears to be working fine now. If it happens again I will get a project together; but my feeling is that something was wonky when I put this up and now it seems that everything is happy. Okay to close this for now and I will re-open if it burps again?

@twobitunicorn
Copy link
Author

@entioentio You have this issue also? Any chance for a reproduce?

@twobitunicorn twobitunicorn reopened this Mar 19, 2021
@entioentio
Copy link

entioentio commented Mar 19, 2021

@twobitunicorn this is not much more than a reproduction case https://github.com/entioentio/toodoo. Repo made for playing with faunaDB addon to netlify and that all with vite + vue. Weird as it is, it happens only with $ netlify dev, seems to work when deployed. I guess it has something to do with my local setup then. It was really late for me yesterday, sorry if it doesn't make much sense. I'll make a new project using this stack again and check it when I have some time.

@twobitunicorn
Copy link
Author

@entioentio Thanks. I was also able to reproduce it here: https://github.com/twobitunicorn/globalThis
if we do npm install --save faunadb we will get the ReferenceError: require is not defined error. Adding globalThis and modifying the command npm install --save-dev https://github.com/twobitunicorn/faunadb-js/tarball/master seems to do the trick.

@fireridlle
Copy link
Contributor

@entioentio thank you for the repository. I was able to run netlify dev successfully at my end.

◈ Netlify Dev ◈
◈ Injected netlify.toml file env var: NODE_VERSION
◈ Ignored general context env var: LANG (defined in process)
◈ Overriding command with setting derived from netlify.toml [dev] block: yarn vite
◈ Functions server is listening on 52068
◈ Starting Netlify Dev with #custom
yarn run v1.22.10
warning package.json: No license field
$ /Users/szinkevych/projects/faunadb/globalThisRepo/node_modules/.bin/vite

  vite v2.0.5 dev server running at:

  > Local:    http://localhost:3005/
  > Network:  http://192.168.0.102:3005/

  ready in 361ms.

@fireridlle
Copy link
Contributor

@twobitunicorn thank you for the repository. I was able to reproduce the issue with global not found.
PR with a cross-platform fix is created.

After fixing this, I have got require is not defined. seems like both are Svelte issues as none of them are reproducible with react-create-app and other frameworks. Basically, Svelte doesn't support CommonJS, and not the only faunadb that doesn't work with Svelte but also some other package (example: axios).
You can refer to their repo for help, here is some start point to research sveltejs/svelte#4775

@fireridlle fireridlle closed this Mar 31, 2021
@entioentio
Copy link

entioentio commented Mar 31, 2021

@fireridlle I've just realized that I have pushed my silly workaround for the time being (I'm not even sure why the heck should it work 🤷‍♂️ window.global = window) 🤦

👉 https://github.com/entioentio/toodoo/blob/e79128e3c9f8c2661076abae09c8be149072a833/src/api/index.js#L4

I've fresh installed it and still get the error. I hate to ask you this since you've already spent much time om this, but could please check to reproduce this withouth this line? It's already gone from the repo.

@fireridlle
Copy link
Contributor

With removing window.global = window I've got require is not defined.
The issue is the same as for Svelte
vitejs/vite#2161
vitejs/vite#2618
vitejs/vite#728

BTW, we are working to provide both CJS and ES5 modules systems. The release date is not clear yet.

FYI, I found another issue with vitejs, the doesn't respect browser field at package.json. here is issue i've created
vitejs/vite#2802

@twobitunicorn
Copy link
Author

@fireridlle Rad. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants