Skip to content

Conversation

samcooke98
Copy link

Summary

I was experimenting with the new Server components. I noticed that this didn't work in webpack 5 due to the use of some deprecated APIs.

Additionally, I noticed the regex was wrong (c3f621d), or at least it wouldn't include tsx files as server components.

I totally understand if you only wish to support webpack v4 while you experiment with Server-components. However, I thought this PR would be useful when you decide to support webpack v5 - This is why I'm opening it as a draft PR.

Test Plan

I added a webpack 5 alias. I wrote a test fixture in ReactFlightWebpackPlugin-test.js. This tests ensure that the webpack compilation:

  • Outputs the manifest, with the expected result
  • Completes the compilation with no errors.

- This commit adds tests running in both webpack v4 and webpack v5.

- Interestingly, both pass - I expected webpack v5 too fail.

Will investigate
@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 61d3dc8:

Sandbox Source
React Configuration

Copy link
Author

@samcooke98 samcooke98 left a comment

Choose a reason for hiding this comment

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

This is a quick, non complete self review. I'll do a more indepth one before making this a proper PR, if there is appetite for this PR

"replace-fork": "node ./scripts/merge-fork/replace-fork.js"
},
"dependencies": {
"webpack5": "npm:webpack@5"
Copy link
Author

Choose a reason for hiding this comment

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

Oops, this should be a devDependency.

}
};

//
Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
//

// TODO: Hook into deps instead of the target module.
// That way we know by the type of dep whether to include.
// It also resolves conflicts when the same module is in multiple chunks.
if (!/\.client\.(tsx|jsx|js|ts)$/.test(resource)) {
Copy link
Author

Choose a reason for hiding this comment

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

From reading the RFC, I think the valid extensions should be any file that webpack accepts? EG: resolve.extensions

In other words, from my undertanding of the RFCS: This code should be valid.

// // Parent.server.js

import Component from "Component.client.js";
import implA from "implA.client.js";
import Picture from "MyCatPhoto.client.png";

export default function Parent() {
  return <Component avatar={Picture} impl={implA} />
}

(from: https://github.com/reactjs/rfcs/blob/235f27c12aa893efd2378ec3c4a9b0b221641861/text/0000-server-module-conventions.md#implicit-client-references

compilation.hooks.processAssets.tap(
{
name: PLUGIN_NAME,
// We derive the manifest from the existing assets.
Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
// We derive the manifest from the existing assets.

I don't think this comment adds anything

@stale
Copy link

stale bot commented Jun 11, 2021

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jun 11, 2021
@gaearon
Copy link
Collaborator

gaearon commented Nov 25, 2021

Thanks for the early exploration! We merged #22739 which updates to Webpack 5 only. If you see something there that's wrong (and that your PR covers), please let me know!

The "automated test" part of this PR is interesting. We're going v5+ only, but I'm curious if you'd like to resubmit this part as a separate PR. Note that we wouldn't want to hardcode the output — but a snapshot test to prevent regressions seems useful.

@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Nov 25, 2021
@samcooke98
Copy link
Author

The "automated test" part of this PR is interesting. We're going v5+ only, but I'm curious if you'd like to resubmit this part as a separate PR. Note that we wouldn't want to hardcode the output — but a snapshot test to prevent regressions seems useful.

Happy to resubmit that in a separate PR, with a snapshot test instead. Will open one shortly, and close this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants