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

Implement polyfill for globalThis #4653

Merged
merged 7 commits into from
Jun 23, 2022
Merged

Implement polyfill for globalThis #4653

merged 7 commits into from
Jun 23, 2022

Conversation

tomduncalf
Copy link
Contributor

@tomduncalf tomduncalf commented Jun 16, 2022

What, How & Why?

This closes #4350. I decided to replace the existing inline implementations with a more robust polyfill taken from core-js, and to put it in realm.io/common so we don't need to always repeat the same snippet

☑️ ToDos

  • 📝 Changelog entry
  • 📝 Compatibility label is updated or copied from previous entry
  • 🚦 Tests
  • 🔀 Executed flexible sync tests locally if modifying flexible sync
  • 📱 Check the React Native/other sample apps work if necessary
  • 📝 Public documentation PR created or is not necessary
  • 💥 Breaking label has been applied or is not necessary

If this PR adds or changes public API's:

  • typescript definitions file is updated
  • jsdoc files updated
  • Chrome debug API is updated if API is available on React Native

@@ -181,6 +181,7 @@ const Person = {
* Logging out too quickly can cause an error if the timeout behavior is set to `openLocalRealm` ([#4453](https://github.com/realm/realm-js/issues/4453), since v10.0.0)
* Released `realm-network-transport` to adopt the changes published to fix `globalThis` undefined issue for older devices. ([#4350](https://github.com/realm/realm-js/issues/4350), since v10.0.0)
* Fixed flexible sync crash when updating subscriptions after token expiry. ([#4421](https://github.com/realm/realm-js/issues/4421), since v10.12.0)
* Fixed remaining uses of `globalThis` undefined issue, causing Realm to not load on iOS 11/12. ([#4350](https://github.com/realm/realm-js/issues/4350))
Copy link
Contributor

Choose a reason for hiding this comment

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

:shipit: Do we know if it has always been an issue - or was the bug introduced in a specific version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, I will check the git history and see if I can tell

Copy link
Member

@kraenhansen kraenhansen left a comment

Choose a reason for hiding this comment

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

I'm wondering if we could simplify the solution, but simply documenting the use of the @babel/preset-env Babel preset?

@tomduncalf
Copy link
Contributor Author

I'm wondering if we could simplify the solution, but simply documenting the use of the @babel/preset-env Babel preset?

As in we say to users that they need to use @babel/preset-env in their project if they are on an old iOS version?

@kraenhansen
Copy link
Member

kraenhansen commented Jun 17, 2022

As in we say to users that they need to use @babel/preset-env in their project if they are on an old iOS version?

Yeah. Or perhaps https://www.npmjs.com/package/babel-plugin-transform-globalthis? Since I don't see react-native in the supported targets of @babel/preset-env and it doesn't see like the metro-react-native-babel-preset is using it as is.

@kraenhansen
Copy link
Member

kraenhansen commented Jun 17, 2022

Regardless: If we choose to discourage the explicit usage of globalThis, I think this PR should ensure eslint errors on that.

@tomduncalf
Copy link
Contributor Author

I was following the pattern of what we've already done for globalThis in a couple of places, which is to fix our library so that it works with all supported OS versions. I think I prefer that approach (we do the nasty stuff so users don't have to do any workarounds) but I am also happy if the majority would prefer to ask users to install a Babel plugin for this reasonably edge case.

@kneth @takameyer any thoughts?

Good suggestion re: the lint rule

@kraenhansen
Copy link
Member

we do the nasty stuff so users don't have to do any workarounds

I agree - and ideally we would apply this in a build step before publishing our package for consumption.
I'd expect this to be easier to implement in the future as I plan on adding rollup to the package as part of the binding generator project.

@tomduncalf
Copy link
Contributor Author

tomduncalf commented Jun 17, 2022

we would apply this in a build step before publishing our package for consumption.

Got it, this would be the ideal (we use the Babel plugin on our code before publish). In the interim until we have the infra in place to do that (or can we do it right now?), are you OK with this solution?

@takameyer
Copy link
Contributor

takameyer commented Jun 20, 2022

any thoughts?

I think we should do the work for our users here. If it's possible that we implement the babel plugin in our own codebase, I would prefer this.

Copy link
Contributor

@takameyer takameyer left a comment

Choose a reason for hiding this comment

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

LGTM

@kneth
Copy link
Contributor

kneth commented Jun 20, 2022

any thoughts?

I agree with @takameyer

@tomduncalf
Copy link
Contributor Author

@tomduncalf Add eslint rule

@tomduncalf
Copy link
Contributor Author

@tomduncalf Plan: merge this PR, rebase v11 against master, rebase v11 PR to remove cherrypick commit and merge

@tomduncalf
Copy link
Contributor Author

@kraenhansen I added the eslint rule in all public facing packages: b92a7f8

@tomduncalf
Copy link
Contributor Author

If everyone is happy with this, I'll publish the new common version and rerun CI

@kraenhansen
Copy link
Member

I added the eslint rule in all public facing packages: b92a7f8

I think it should be enough to add it to the root package and have the other packages inherit?

@tomduncalf
Copy link
Contributor Author

I think it should be enough to add it to the root package and have the other packages inherit?

Ah I didn't realist that they inherited! Even better, I'll update

@tomduncalf tomduncalf closed this Jun 21, 2022
@tomduncalf tomduncalf reopened this Jun 21, 2022
@tomduncalf tomduncalf merged commit 893b32e into master Jun 23, 2022
@tomduncalf tomduncalf deleted the td/globalthis branch June 23, 2022 14:12
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iOS - Can't find variable: globalThis
4 participants