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

Add globalThis.fetch #934

Conversation

takurinton
Copy link
Contributor

I had this conversation with @developit on Twitter.
https://twitter.com/_developit/status/1558610677157269504

wmr gives an error if there is no fetch when prerendering.
To work around this, define globalThis.fetch in preact-iso's prerender.

@changeset-bot
Copy link

changeset-bot bot commented Aug 14, 2022

🦋 Changeset detected

Latest commit: c0b0b1b

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

This PR includes changesets to release 1 package
Name Type
wmr Minor

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

@takurinton
Copy link
Contributor Author

@developit

I am wondering if I should include the following DOMParser in the init() function. What do you think?

globalThis.DOMParser = new (require("jsdom").JSDOM)("").window.DOMParser;

Copy link
Member

@rschristian rschristian left a comment

Choose a reason for hiding this comment

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

This should be done in WMR's prerender rather than preact-iso: https://github.com/preactjs/wmr/blob/main/packages/wmr/src/lib/prerender.js

We set a couple other things on globalThis there, as you can see.

Edit: To expand a little, preact-iso isn't tied to WMR. It very well could be used in other environments. As such, if we want to fix a shortcoming in WMR specifically, we'll want to move this into WMR's prerender script.

This also allows us to pass config values, like the output directory, easily.

packages/preact-iso/prerender.js Outdated Show resolved Hide resolved
@takurinton
Copy link
Contributor Author

takurinton commented Aug 14, 2022

@rschristian

To expand a little, preact-iso isn't tied to WMR.

Okay, I was a little mistaken. I thought this was all inclusive.
And I have completed porting globalThis.fetch to the wmr code and have confirmed that it also passes CI.

@rschristian
Copy link
Member

Totally fair! To be honest it's in a bit of a weird spot right now. We'd like people to use it for more than just WMR, but, being buried in this monorepo, a lot of people don't even know it exists (we're working on fixing that though).

Could we add a test case for this? Super simple fetch for markdown, like you do in your blog, would be awesome!

You can add a test fixture here: https://github.com/preactjs/wmr/tree/main/packages/wmr/test/fixtures
And a test case here: https://github.com/preactjs/wmr/blob/main/packages/wmr/test/production.test.js#L762

Just need to build & prerender the fixture then check that the prerendered HTML contains our markdown content.

@takurinton
Copy link
Contributor Author

@rschristian
I'm new to wmr core and still don't have the full picture and completely forgot about tests.
I'll get to it right away!! Thank you!!

packages/wmr/test/production.test.js Outdated Show resolved Hide resolved
@takurinton
Copy link
Contributor Author

takurinton commented Aug 15, 2022

@rschristian
One test is failing, but the rest are passing. All tests passed!!
Can this PR be merged? Also is there anything else I can do? Thank you.

@rschristian
Copy link
Member

rschristian commented Aug 16, 2022

Nearly forgot, needs a changeset.

You can run yarn changeset from the root of the repo to open a CLI for creating a changeset, which is a description of the change the PR provides (goes into forming the changelog). I'd probably mark this as a minor due to the added functionality.

I can get around to doing this later & merging too.

Edit: Indeed, we seem to have a flakey test. Took me a few reruns before everything passed without issue.

@takurinton
Copy link
Contributor Author

@rschristian
Thanks for the changeset change!
I'm at work right now and wasn't able to respond. Appreciate it!

@rschristian rschristian merged commit 6a9d776 into preactjs:main Aug 16, 2022
@rschristian
Copy link
Member

Thanks!

@github-actions github-actions bot mentioned this pull request Aug 16, 2022
@danielweck
Copy link

Indeed, we seem to have a flakey test. Took me a few reruns before everything passed without issue.

Possibly related?

@rschristian
Copy link
Member

@danielweck Actually no, I haven't seen segfaults in ages. Looks to be fixed on Node's side? Not sure if others share that experience or I somehow got lucky.

The flakey tests here were unrelated. HMR issues.

@danielweck
Copy link

Thanks @rschristian I'll check my Node / segfault status.

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