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

fix: Last, not first, non-external script in HTML should have prerender() #905

Merged
merged 3 commits into from
Mar 30, 2022

Conversation

rschristian
Copy link
Member

@rschristian rschristian commented Jan 24, 2022

The first script tag in the file should be assumed to have a prerender(), however, due to the loop continuing even after the first non-external script tag is found, it was actually the last script tag that was used and would need to have prerender().

It should be noted that this behavior has existed for the past 9 months, and fixing it could potentially cause someone's setup to break. We could just flip the error message/comments instead?

throw Error(`No prerender() function was exported by the first <script src="..."> in your index.html.`);

@changeset-bot
Copy link

changeset-bot bot commented Jan 24, 2022

🦋 Changeset detected

Latest commit: 4494e19

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

This PR includes changesets to release 1 package
Name Type
wmr 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

@developit
Copy link
Member

oooof, yeah. TBH that's a solid argument in favor of flipping the error message. I can think of cases where first script is likely not the correct one (polyfills), but I can also think of cases where the last script is incorrect (analytics).

@github-actions
Copy link
Contributor

Size Change: +4 B (0%)

Total Size: 806 kB

Filename Size Change
packages/wmr/wmr.cjs 767 kB +4 B (0%)
ℹ️ View Unchanged
Filename Size
examples/demo/dist/about/index.html 721 B
examples/demo/dist/alias-outside/index.html 697 B
examples/demo/dist/assets/Calendar.********.css 694 B
examples/demo/dist/assets/style.********.css 624 B
examples/demo/dist/chunks/alias-outside.********.js 138 B
examples/demo/dist/chunks/class-fields.********.js 211 B
examples/demo/dist/chunks/compat.********.js 15.7 kB
examples/demo/dist/chunks/hoofd.module.********.js 1.48 kB
examples/demo/dist/chunks/index.********.js 201 B
examples/demo/dist/chunks/json.********.js 239 B
examples/demo/dist/chunks/meta-tags.********.js 297 B
examples/demo/dist/chunks/prerender.********.js 2.47 kB
examples/demo/dist/class-fields/index.html 709 B
examples/demo/dist/compat/index.html 1.62 kB
examples/demo/dist/env/index.html 783 B
examples/demo/dist/error/index.html 714 B
examples/demo/dist/files/index.html 744 B
examples/demo/dist/index.********.js 7.8 kB
examples/demo/dist/index.html 775 B
examples/demo/dist/json/index.html 719 B
examples/demo/dist/lazy-and-late/index.html 723 B
examples/demo/dist/meta-tags/index.html 789 B

compressed-size-action

@rschristian
Copy link
Member Author

Will do that instead (only thought of it after I pushed, whoops).

Analytics will probably end up being external more often than not, ensuring they get skipped. So long as the error messages matches the behavior it should be an easy and quick correction anyways.

@rschristian rschristian changed the title fix: First, not last, non-external script in HTML should have prerender() fix: Last, not first, non-external script in HTML should have prerender() Jan 24, 2022
@developit developit merged commit 11663db into main Mar 30, 2022
@developit developit deleted the fix/script-tag-prerender branch March 30, 2022 20:03
@github-actions github-actions bot mentioned this pull request Mar 30, 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.

3 participants