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

Strict check on browser side code #137

Merged
merged 4 commits into from
Nov 2, 2023
Merged

Conversation

tom-quiltt
Copy link
Contributor

@tom-quiltt tom-quiltt commented Nov 1, 2023

Turns out expo has window but doesn't have window.addEventListener.
So when I import @quiltt/core in the react native sdk and use it in an expo app, it just blows up.

 console.log('window.expo', !!window.expo)
 LOG  window.expo true

I have a hard time to just use @quiltt/core directly in the expo example app, so hoping to add one more check, bump the version so I can use the next @quiltt/core directly in the expo example app.

In node

❯ node
Welcome to Node.js v18.15.0.
Type ".help" for more information.
> typeof window !== 'undefined' && !window?.expo
false

In browser

typeof window !== 'undefined' && !window?.expo
true

In Expo

console.log('in expo', typeof window !== 'undefined' && !window?.expo)

 LOG  in expo false

Copy link

changeset-bot bot commented Nov 1, 2023

🦋 Changeset detected

Latest commit: 5adb449

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@quiltt/core Patch
@quiltt/react Patch
@quiltt/react-test-nextjs Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

ECMAScript/core/src/types.ts Show resolved Hide resolved
.changeset/brown-onions-carry.md Outdated Show resolved Hide resolved
@tom-quiltt
Copy link
Contributor Author

@zubairaziz would love your review and expertise on how to skip expo app check, can revert if you see a problem.

@tom-quiltt tom-quiltt merged commit ab55ccb into main Nov 2, 2023
4 checks passed
@tom-quiltt tom-quiltt deleted the check-against-expo-app branch November 2, 2023 00:53
@zubairaziz
Copy link
Collaborator

@zubairaziz would love your review and expertise on how to skip expo app check, can revert if you see a problem.

I don't see anything wrong with what you're doing here, but is there anywhere we can get the types for expo so that we can maybe have:

declare global {
  interface Window {
    expo?: ExpoType
  }
}

instead of:

declare global {
  interface Window {
    expo: any
  }
}

@tom-quiltt
Copy link
Contributor Author

where can I get ExpoType, assuming it's from some Expo pacakge, feel a bit weird to have core to depends on a react native package.

@zubairaziz
Copy link
Collaborator

Yeah, I'm not sure. I just used ExpoType as an example, but I'm sure this is a common enough issue that someone probably has this typed somewhere already

@tom-quiltt
Copy link
Contributor Author

I will check it again if we need it then, right now we are not even accessing actual value of the expo window object, just want find a way to skip it.

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