Preserve "use client" module boundaries
#516
Conversation
🦋 Changeset detectedLatest commit: 0d24b15 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #516 +/- ##
==========================================
- Coverage 90.90% 90.82% -0.08%
==========================================
Files 33 34 +1
Lines 1495 1537 +42
Branches 422 425 +3
==========================================
+ Hits 1359 1396 +37
- Misses 130 135 +5
Partials 6 6
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
| if (!output.meta) { | ||
| output.meta = {}; | ||
| } | ||
| output.meta.isUseClientEntry = true; |
There was a problem hiding this comment.
perhaps it would be a good idea to prefix this property with something like _preconstruct_?
There was a problem hiding this comment.
We control all the Rollup plugins used so it doesn't really matter
There was a problem hiding this comment.
Ah, right - good point. I forgot that this isn't anyhow exposed.
| const moduleInfo = this.getModuleInfo(chunk.facadeModuleId); | ||
| if (moduleInfo?.meta.isUseClientEntry) { | ||
| const magicString = new MagicString(code); | ||
| magicString.prepend('"use client";\n'); |
There was a problem hiding this comment.
i guess this is because Rollup might drop the directive, right?
There was a problem hiding this comment.
Rollup actually throws a warning if you have directives so this removes the directive and then adds it back.
There was a problem hiding this comment.
Do u know if that's also the case in Rollup 3 (IIRC we are still on Rollup 2 here)?
There was a problem hiding this comment.
There was a problem hiding this comment.
Interesting, it might be worth creating a Rollup issue about this. I suspect that more people might need to handle those directives in the near-ish future.
There was a problem hiding this comment.
tbh, imo the current behaviour is the most sensible option. There's no correct answer to bundling directives so erroring on it seems like the ideal thing.
| } | ||
| if (node.expression.value === "use client") { | ||
| let magicString = new MagicString(code); | ||
| this.emitFile({ type: "chunk", id }); |
There was a problem hiding this comment.
I guess this is the core of the change that allows Rollup to preserve this file as a separate chunk, right?
| "src/b.js": js` | ||
| export const B = "b"; | ||
| `, | ||
| "src/c.js": js` | ||
| import { D } from "./d"; | ||
| export function C() { | ||
| return D; | ||
| } | ||
| `, | ||
| "src/d.js": js` | ||
| "use client"; | ||
| export const D = "d"; | ||
| `, |
There was a problem hiding this comment.
those end up being unused - was this intentional?
There was a problem hiding this comment.
No, need to fix that, it was from debugging
Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
emmatown
force-pushed
the
server-components
branch
from
e1435e1
to
0d24b15
Compare
No description provided.