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

Update useScript.ts #197

Closed
wants to merge 4 commits into from
Closed

Update useScript.ts #197

wants to merge 4 commits into from

Conversation

KATT
Copy link

@KATT KATT commented Aug 29, 2022

  • Make it so we get initial load status on first mount
  • Add cachedScriptStatuses-record for short-circuting & avoiding DOM-lookup

Potentially, we could have a global Record<string, string|undefined> defined rather than relying on querySelector to make it faster.

KATT added 3 commits August 29, 2022 10:25
Make it so we get initial load status on first mount


Potentially, we could have a global `Record<string, string|undefined>` defined rather than relying on `querySelector` to make it faster.
Comment on lines -11 to -13
if (!src) {
setStatus('idle')
return
Copy link
Author

Choose a reason for hiding this comment

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

Deleted this, it felt weird to have an implicit '' as an acceptable value, if anything I'd say we should do string | null

@juliencrn
Copy link
Owner

Hi @KATT,

Thanks for this interesting PR, unfortunately the repository structure have been changed since you've opened your PR.
I've created a new one using your code. I'll add your PR ID and your name in the commit message and in the release note.

Please, could you review this change?

@juliencrn juliencrn closed this Sep 30, 2022
juliencrn added a commit that referenced this pull request Oct 13, 2022
juliencrn added a commit that referenced this pull request Oct 13, 2022
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