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

[legacy-framework] Change db/index.ts in new app template to use globalThis instead of global #813

Merged
merged 6 commits into from Aug 16, 2020

Conversation

kripod
Copy link

@kripod kripod commented Jul 30, 2020

What are the changes and their implications?

While Node.js has a non-standard global namespace object, version 12.4 introduced support for the standard globalThis object. Blitz requires Node.js >=12, so the change could be made without losing backwards compatibility.

Checklist

  • Tests added for changes – N/A
  • PR submitted to blitzjs.com for any user facing changes – N/A

@kripod kripod requested a review from flybayer as a code owner July 30, 2020 11:53
@blitzjs-bot blitzjs-bot bot added this to In Review in Dashboard Jul 30, 2020
Copy link

@merelinguist merelinguist left a comment

Choose a reason for hiding this comment

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

Thanks again, @kripod! Could you have a look at the lint errors?

@kripod
Copy link
Author

kripod commented Jul 30, 2020

Sure! It turns out that only ESLint >=7 supports globalThis, with the built-in ES2020 parser. As a workaround, please consider adding the following code to the ESLint config:

{
  "globals": {
    "globalThis": false // Read-only
  }
}

@flybayer
Copy link
Member

flybayer commented Aug 2, 2020

Hey thanks for the PR @kripod!!

I just did some digging and eslint-config-react-app just added support for eslint v7 relatively recently. So that means we can upgrade everything to v7 and then switch to globalThis.

I opened an issue for upgrade eslint. You're welcome to take that or let someone else. Once that issue is finished, then we can merge this.

However, for this PR you also need to change global to globalThis in all the other examples in the repo and in the new app template

@kripod
Copy link
Author

kripod commented Aug 2, 2020

Thanks for getting back to me!

Unfortunately, I’m not quite sure whether I’ll have time to do all of this.

@kripod kripod requested a review from aem as a code owner August 5, 2020 09:10
@merelinguist
Copy link

@kripod No worries, I’ve finished up the rest for you. All that remains is for blitz-js/legacy-framework#661 to be closed, and then this can be merged.

@merelinguist merelinguist added the status/blocked Waiting on something else label Aug 5, 2020
@blitzjs-bot blitzjs-bot bot moved this from In Review to Blocked in Dashboard Aug 5, 2020
merelinguist
merelinguist previously approved these changes Aug 5, 2020
@kripod
Copy link
Author

kripod commented Aug 5, 2020

@merelinguist Wonderful, thank you! 🙌

@flybayer
Copy link
Member

Hey @kripod! We finally upgrade to eslint v7! So now we have one final issue it seems. Apparently we also have to add es2020 as the eslint environment. This needs added to the example apps and to the new app template eslint config file.

@kripod
Copy link
Author

kripod commented Aug 13, 2020

Wonderful, thank you! Unfortunately, I’m currently busy with a company projects, so I may not have time to complete the task…

@flybayer
Copy link
Member

Ok no problem!

@wKovacs64 wKovacs64 force-pushed the refactor-global-to-globalThis branch from 445a99f to d376c1a Compare August 15, 2020 00:27
@wKovacs64
Copy link
Collaborator

Not sure what the implications are (if any) of enabling es2020 in the TS projects, but I added it to the app template and all examples.

@wKovacs64 wKovacs64 added status/in-review and removed status/blocked Waiting on something else labels Aug 15, 2020
@blitzjs-bot blitzjs-bot bot moved this from Blocked to In Review in Dashboard Aug 15, 2020
@flybayer
Copy link
Member

Perfect, thanks @kripod and @wKovacs64!!

@all-contributors add @kripod for code

@allcontributors
Copy link

@flybayer

I've put up a pull request to add @kripod! 🎉

@flybayer flybayer changed the title refactor: use standard 'globalThis' object in Node Change db/index.ts in new app template to use globalThis instead of global Aug 16, 2020
@flybayer flybayer merged commit fc68175 into blitz-js:canary Aug 16, 2020
Dashboard automation moved this from In Review to Done Aug 16, 2020
@kripod kripod deleted the refactor-global-to-globalThis branch August 16, 2020 14:07
@dillondotzip dillondotzip changed the title Change db/index.ts in new app template to use globalThis instead of global [legacy-framework] Change db/index.ts in new app template to use globalThis instead of global Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants