Skip to content

fix: added check for document, so redaxios will not break in Node#57

Merged
developit merged 2 commits intodevelopit:masterfrom
AlexandrHoroshih:node-fix
Oct 9, 2020
Merged

fix: added check for document, so redaxios will not break in Node#57
developit merged 2 commits intodevelopit:masterfrom
AlexandrHoroshih:node-fix

Conversation

@AlexandrHoroshih
Copy link
Copy Markdown
Contributor

Hi!
I'm currently using redaxios in a project with Next.js and found this problem: using redaxios in Node.js leads to ReferenceError because there is no document object in Node.

Axios for this case provides noop functions for non-browser environments: https://github.com/axios/axios/blob/04d45f20911a02e9457db9e9d104aa156e170b11/lib/helpers/cookies.js#L45.

I added a check on the document object so that code with redaxios can be used in the Node.js environment too

@developit, can you review, please?

@googlebot
Copy link
Copy Markdown
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@AlexandrHoroshih
Copy link
Copy Markdown
Contributor Author

@googlebot I signed it!

@googlebot
Copy link
Copy Markdown
Collaborator

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@developit
Copy link
Copy Markdown
Owner

Hi there - this doesn't make redaxios work in Node, it just makes it not error out. You should be using Axios in Node, and only installing the Redaxios alias for browser compilation. In Next you can do this by checking env.ssr.

@AlexandrHoroshih
Copy link
Copy Markdown
Contributor Author

AlexandrHoroshih commented Sep 22, 2020

@developit Sorry, I'll correct myself:
this will allow redaxios to be used in Next.js, which adds a server side Fetch polyfill (https://nextjs.org/docs/basic-features/supported-browsers-features#polyfills) or in any other Node application with a polyfill for Fetch, or a custom implementation added via the redaxios config.

I think this will greatly simplify the use of redaxios with Next.js

@developit
Copy link
Copy Markdown
Owner

Ah! Yes I actually helped with the fetch polyfill, I should have thought of that!

@AlexandrHoroshih
Copy link
Copy Markdown
Contributor Author

AlexandrHoroshih commented Oct 1, 2020

So, what you think, @developit?

This line of code, where i had added check for document - is the only line that fails in Next.js (i tried it with my forked version).
Being able to use redaxios in any environment that supports Fetch API (like Next.js server-side part) - looks good to me

@developit
Copy link
Copy Markdown
Owner

Hey, sorry for dropping the ball on this. I'm okay with the change, just wanted to check on some size tweaks. Hopefully this week!

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