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.

Sign up for GitHub

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

Jump to bottom

Make ss rnode safe #6

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

Make ss rnode safe #6

Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e56953c
add isBroswer varable
mrhut10 Apr 8, 2020
fbee62c
make browser safe
mrhut10 Apr 8, 2020
aa35ecd
remove semicolons
mrhut10 Apr 8, 2020
24c1466
change when isBrowser is checked
mrhut10 Apr 8, 2020
cf3ca57
testing direct replacment of isBrowser with hasIDB
mrhut10 May 11, 2020
81ffbe3
hasIDB dirrect replacement for isBroswer
mrhut10 May 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions idb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
const isBrowser = typeof window !== 'undefined'
mrhut10 marked this conversation as resolved.
Show resolved Hide resolved

const dbp = new Promise((resolve, reject) => {
if (!isBrowser) {
return resolve(undefined)
}
const openreq = window.indexedDB.open('use-idb', 1)
openreq.onerror = () => reject(openreq.error)
openreq.onsuccess = () => resolve(openreq.result)
Expand All @@ -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 •
edited
Loading

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 :)

const db = await dbp
const transaction = db.transaction('idb', type)
const store = transaction.objectStore('idb')

return new Promise((resolve, reject) => {
const req = store[method](...args)
transaction.oncomplete = () => resolve(req)
transaction.onabort = transaction.onerror = () => reject(transaction.error)
if (isBrowser){
const transaction = db.transaction('idb', type)
const store = transaction.objectStore('idb')
const req = store[method](...args)
transaction.oncomplete = () => resolve(req)
transaction.onabort = transaction.onerror = () => reject(transaction.error)
} else {
resolve(undefined)
}
})
}

Expand Down