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

Tests for the RSC shim #6

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

lubieowoce
Copy link
Contributor

Just sketching some stuff out. Not sure if i like the conditional bits... but it's hard to do that otherwise w/o a ton of duplication. for example, useContext seems to exist in the RSC world in the React version we're installing! Lemme know what you think or if you have ideas on how to structure this better.

@phryneas
Copy link
Owner

Hmm, just a thought: might it make sense to directly import the react dist files for the different environments and compare those with the rsc export?

Generally: A little bit of duplication is not a bad thing - too much abstraction makes tests harder to read :)

It's already late, so I'll take a closer look tomorrow!

@lubieowoce
Copy link
Contributor Author

lubieowoce commented Feb 14, 2024

A little bit of duplication is not a bad thing - too much abstraction makes tests harder to read :)

Oh don't worry, the last commit is already slightly de-abstractionized :D the original was worse lol. But lmk which bits you think we should de-abstractionize further, i have a penchant for deduplicating my tests to death lol

might it make sense to directly import the react dist files for the different environments and compare those with the rsc export?

well, the issue is that rsc.js imports react directly. so we can import anything we want in the test, but it might not be what the shim imported, so there's no point. we could use module mocking, but node:test doesn't have that, so we'd need to pull in something that does.

...but also i think it'd be better to stick to plain --conditions, no mocking/resolution tricks. that way we can kinda ensure we're testing the real behavior. we can just have two test scripts, one for RSC, one for vanilla


const missingFactories = ["createContext", "createFactory"];

const missingClasses = ["Component", "PureComponent"];
Copy link
Contributor Author

@lubieowoce lubieowoce Feb 14, 2024

Choose a reason for hiding this comment

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

TODO - tests for classes

@phryneas
Copy link
Owner

phryneas commented Feb 16, 2024

well, the issue is that rsc.js imports react directly. so we can import anything we want in the test, but it might not be what the shim imported, so there's no point. we could use module mocking, but node:test doesn't have that, so we'd need to pull in something that does.

...but also i think it'd be better to stick to plain --conditions, no mocking/resolution tricks. that way we can kinda ensure we're testing the real behavior. we can just have two test scripts, one for RSC, one for vanilla

My idea was somewhere along the lines of

const browserReact = require('react/index.js')
const rscReact = require('react/react.react-server.js')

const rehackt = require('rehackt')

for (const key of Object.keys(browserReact)) {
  assert.ok(key in rehackt)
  assert.equal(typeof rehackt[key], typeof browserReact[key])
  assert.notEqual(rehackt[key], browser[key])
}
for (const key of Object.keys(rscReact)) {
  assert.equal(rehackt[key], rscReact[key])
}

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