preactjs / wmr

👩‍🚀 The tiny all-in-one development tool for modern web apps.
https://wmr.dev/
MIT License
4.92k stars 109 forks source link

Add globalThis.fetch #934

Closed takurinton closed 1 year ago

takurinton commented 1 year ago

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[bot] commented 1 year ago

🦋 Changeset detected

Latest commit: c0b0b1b8df5ca6a650a7dc8f8edad73bf70de52d

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 commented 1 year ago

@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;
takurinton commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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!!

takurinton commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

rschristian commented 1 year ago

Thanks!

danielweck commented 1 year ago

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

Possibly related?

rschristian commented 1 year ago

@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 commented 1 year ago

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