-
Notifications
You must be signed in to change notification settings - Fork 70
fix: remove node_modules guard from entry branching + add playground example #906
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
Conversation
| **/worker-configuration.d.ts | ||
|
|
||
| # Webstorm | ||
| .idea/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can remove this if needed. I didn't find any obvious place for IDEs to place this under
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can keep it!
4e7c6e1 to
2f89162
Compare
| @@ -0,0 +1,3 @@ | |||
| import { initClient } from "rwsdk/client"; | |||
|
|
|||
| initClient(); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so this is what was needed for now - for the monorepo's playground example case because of workspace links - right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it would've been possible to change Document.tsx to link to it via packages/entrypoint/src/client.tsx, but I felt like this illustrated the flow better, since the idea was to use a file in node_modules as the worker entry.
|
Thank you for working on this @Zn4rK! Appreciate you taking time on the E2E test - I know it was a can of worms. Will keep an eye out for when CI is green, let me know if it'd help for me to take a look as well. |
| "./worker": "./src/worker.tsx" | ||
| }, | ||
| "dependencies": { | ||
| "react": "19.3.0-canary-fd524fe0-20251121", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be a dependency to rwsdk in a "real" package.json, but in this monorepo it was very hard to get the e2e test suite to play along. TypeScript doesn't care, because the playground package entrypoint-from-node_modules has that dependency. And vite resolves the packages relative to the (playground) project, so it works without this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. I guess its because in our e2e test setup, we create an isolated directory for the project and install rwsdk as a tarball there. We kind of need this to mimic users' application setup, rather than test in the less realistic scenario where rwsdk is a workspaced-linked package in the same monorepo. I can see why this makes it difficult to set up this playground's example though. It makes sense to leave it out here given this, thanks @Zn4rK
|
Update for posterity: We're skipping the e2e test for this playground example. Our e2e infrastructure runs tests in isolated directories with rwsdk as a tarball (not workspace-linked), which makes testing node_modules scenarios in playground examples difficult. The change works in real-world usage (as confirmed by @Zn4rK), and the e2e tests pass locally, but testing it in our playground setup on CI requires workarounds that aren't worth the rabbit hole to get working right now. We'll revisit e2e testing for monorepo/package scenarios when we have more of these use cases. For context, see the discussion on discord: https://discord.com/channels/679514959968993311/1443579628760334516/1445048899675885608 |
|
Released in https://github.com/redwoodjs/sdk/releases/tag/v1.0.0-beta.34 Thank you @Zn4rK! |
This PR makes two improvements to RedwoodSDK:
Removed the
node_modulesexclusion guard in the entry-resolution logicThe SDK previously had an
ifstatement that stopped the branching logic from running whenever the resolved entry path containednode_modules. This prevented RedwoodSDK from handling entries that legitimately resolve to packages.I removed this
node_modulescheck so that the branching logic always runs, regardless of where the entry is located. This makes the behavior consistent and fixes cases where code insidenode_modulesshould still be treated as an entry point.(Note: in the new playground example, the client entry is not in node_modules; it resides in the playground src due to a symlinking limitation.)
Added a new playground example
I added a minimal example to the
playground/directory based on the existing “Hello World” example. This provides a simple, reproducible environment to test the updated entry-resolution behavior.