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

scripts #30

Closed
wmertens opened this issue Dec 23, 2020 · 7 comments
Closed

scripts #30

wmertens opened this issue Dec 23, 2020 · 7 comments

Comments

@wmertens
Copy link

I'm currently using Helmet but I do prefer the hooks approach you offer.

One thing I'm missing is <script> support. It's nice to be able to encapsulate trackers and other scripts that have to go in <head> for technical reasons so they can be placed where they make sense in the application.

Helmet's support for <script> is very rudimentary IMHO - it just adds the tag without regards for SSR-rendered previous tags, and if you put in an inline script, it won't run it, so that will only work for SSR.

So, ideally, hoofd would:

  • support script tags
  • mark SSR-ed script tags in a way they can be recognized after hydration
  • only add script tags once, also after hydration
  • eval inline scripts when run in the browser, but add them to <head> when run during SSR

But, I would already be happy with simply supporting the tag.

@JoviDeCroock
Copy link
Member

JoviDeCroock commented Dec 23, 2020

Hey,

eval inline scripts when run in the browser, but add them to when run during SSR

Honestly, I don't really feel comfortable allowing arbitrary evaluation or injection of scripts

The other things should be possible, #31

@wmertens
Copy link
Author

The problem is that most trackers require an inline script as well as a regular script. So this solution would not be enough.
Furthermore, you are already allowing arbitrary evaluation of scripts, only they are in the form of hosted scripts instead of inline scripts...

Also, since you're adding the script elements via DOM manipulation, I believe there's no need to eval, the browser will honor the script tag anyway?

Other than that, the PR looks great!

@ggoodman
Copy link
Contributor

I wonder if the inline use-case could be adequately addressed using data-uris, like:

useScript({ src: 'data:application/javascript,alert("yolo")' });

image

@wmertens
Copy link
Author

Great idea!

@JoviDeCroock
Copy link
Member

Closing and pushing #35 and #31 in a 1.2.0 release

@ggoodman
Copy link
Contributor

@JoviDeCroock, should type be marked as optional in ScriptOptions?

@JoviDeCroock
Copy link
Member

Ah damn, forgot about that 😅

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

No branches or pull requests

3 participants