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

Make ss rnode safe #6

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

mrhut10
Copy link

@mrhut10 mrhut10 commented Apr 8, 2020

this is in relation to #5 (window not defined) when using in server-rendered ( SSR )

in my case, I tried to use it in a gatsby project which when the project is built in nodejs the window doesn't exist, hense this libary causes build to fail.

this checks if the window exists then works as normal if the window doesn't exist it will revert to default value.

idb.js Outdated Show resolved Hide resolved
@@ -7,13 +12,16 @@ const dbp = new Promise((resolve, reject) => {

export const call = async (type, method, ...args) => {
Copy link
Owner

Choose a reason for hiding this comment

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

we could have an early return here directly, shouldn't we throw if not supported ?

if (!hasIDB) return reject(Error('indexeDB not found'))

Copy link
Author

Choose a reason for hiding this comment

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

mmm just starting to test this now, although I suspect throwing might lead to SSR failing in practice. I think from a practical point of view if index DB doesn't exist the user should expect just to return the default value. anyway I'll test that assumption now that rejecting will case SSR builds to fail in my codebase at least anyway.

Copy link
Author

Choose a reason for hiding this comment

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

yep just tried this change and it crashes my SSR code, and build code in a gatsby project.

in SSR we if hasIDB is false then it should gracefully degrade to the returning the default value which will make the code SSR safe.

seeing I technically haven't made your requested change I won't hit the resolve button so that you can review it :)

Copy link
Owner

@kigiri kigiri May 12, 2020

Choose a reason for hiding this comment

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

well the idea would be to catch it in your usercode and handle it the way you want. Is that a problem ?

Copy link
Author

Choose a reason for hiding this comment

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

Interesting, before this PR my code base would crash even just from importing it (unless you assigned it with a conditional require statement).

So... We can leave error checking to user but needs to at least not fail on import. I'll double check that's working in my code base later tonight for you. :)

Copy link
Author

Choose a reason for hiding this comment

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

ok just to confirm if we reject here because this promise runs straight away on import it will cause any SSR code to error just by importing it. (unless you instead assign it with a conditional require statement)

import { useIdb } from 'react-use-idb'   /// this will crash SSR code

// alternatively you could use a conditional require statement
const useIdb = typeof window !== 'undefined' ? require('react-use-idb') : null;

@kigiri which Option Do you want me to run with to Resolve?

  1. (assuming you prefer to pass error to user code then
  • delay execution of that promise to when the main method is called?
  • or any other ideas?
  1. (alternatively)
  • gracefully degrade to default if window or indexedDB isn't defined

I'll keep playing to see if another way :)

@mrhut10
Copy link
Author

mrhut10 commented May 9, 2020

wont have time today, but can test your idea tomorrow and confirm works :)

@mrhut10
Copy link
Author

mrhut10 commented May 11, 2020

Ok @kigiri

  • made the change 1 of 2 which worked as a direct replacement.
  • can't make the 2nd change as will make SSR fail again

let me know if you want any other changes / ideas

Regards
Alistair

@mrhut10
Copy link
Author

mrhut10 commented May 12, 2020

Just to clarify we can leave error checking to user although we should document an example of that in the readme.

I'll check later tonight that when SSR is running that it at least doesn't fail just by importing it. (As it did before this PR, i think we've already fixed in this PR but want to double check)

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.

2 participants