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

Webpack 5 for client mono-repo #10186

Merged
merged 9 commits into from
May 10, 2022
Merged

Conversation

CraigMacomber
Copy link
Contributor

@CraigMacomber CraigMacomber commented May 6, 2022

Updates to webpack 5 for #9568

There will be some cleanup after this update to further update webpack-dev-server, and cleanup some no longer needed things.

@github-actions github-actions bot added area: dds: propertydds area: examples Changes that focus on our examples area: loader Loader related issues area: tests Tests to add, test infrastructure improvements, etc dependencies Pull requests that update a dependency file labels May 6, 2022
@CraigMacomber
Copy link
Contributor Author

Still needs a lot of testing and likely fixing, but at least the build passes, and one example (spaces) is known to be working.

@github-actions github-actions bot added the base: main PRs targeted against main branch label May 6, 2022
@CraigMacomber
Copy link
Contributor Author

Test should be passing now, but more manual testing is needed. Some examples have been tested and are working, but more should be tested as well.

@CraigMacomber CraigMacomber marked this pull request as ready for review May 10, 2022 02:41
@CraigMacomber CraigMacomber requested review from msfluid-bot and a team as code owners May 10, 2022 02:41
@msfluid-bot
Copy link
Collaborator

@fluid-example/bundle-size-tests: -1.93 MB
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 400.07 KB 23 Bytes -400.05 KB
containerRuntime.js 221 KB 23 Bytes -220.98 KB
loader.js 160.71 KB 23 Bytes -160.69 KB
map.js 236.1 KB 23 Bytes -236.08 KB
matrix.js 325.26 KB 23 Bytes -325.23 KB
odspDriver.js 182.57 KB 23 Bytes -182.55 KB
odspPrefetchSnapshot.js 77.23 KB 23 Bytes -77.21 KB
sharedString.js 345.14 KB 23 Bytes -345.12 KB
Total Size 1.93 MB 184 Bytes -1.93 MB

Baseline commit: d112cfb

Generated by 🚫 dangerJS against 273f3ef

Copy link
Contributor

@anthony-murphy anthony-murphy left a comment

Choose a reason for hiding this comment

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

awesome! there is one place we use webpack in the server. it's a test to ensure we can webpack local-server for demos and testing: fluid\server\routerlicious\packages\local-server\src\test\isomorphic.spec.ts

Doesn't need to be part of this PR, but would be good to get that updated as well. my guess is that is where the process dependency in many of our demos comes from

"jest": "^26.6.3",
"jest-junit": "^10.0.0",
"jest-puppeteer": "^4.3.0",
"process": "^0.11.10",
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to track down why this polyfill is needed, so I can update #9508 if needed. Do you know where it's coming from?

I think our goal should be that our packages are usable in Webpack without adding polyfills to your config. As of now the plan is to disallow node polyfills except for events and url, but that goal is really focused on our libs. Examples could continue to use polyfills directly in Webpack like you do here, but that implies that our customers will too.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see now in a comment later:

We do however transitively depend on the util npm package (node_modules/util/util.js) which requires process.env to be defined. browserify/node-util#57 (comment)

I'll file an issue if you haven't already. At the very least this will need to be documented when using our packages with Webpack 5.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have not filed any issues about this. To me this looks like a bug in util (has an undeclared dependency), and a quality issue with whatever we use that depends on util (which would be nice for it not to depend on nodejs stuff). I'll leave how to track/address this to you.

When using webpack, I generally expect random transitive dependencies to need manual fiddling (it makes me sad this is something I have accepted), but I suppose trying to get this fixed could help our consumers and may be worth it.

Copy link
Member

Choose a reason for hiding this comment

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

I added #10225.

"jest": "^26.6.3",
"jest-junit": "^10.0.0",
"jest-puppeteer": "^4.3.0",
"process": "^0.11.10",
Copy link
Member

Choose a reason for hiding this comment

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

I'm somewhat surprised you didn't have to polyfill additional node libraries, especially events (#9509) and url (#9510). Do you know why that is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any polyfill that is transitively depended on some other way (like util, which is why we need process) is actually already being included and I don't know if that includes events and url, but it is possible.

It's also quite possible some behaviors are broken, and they were just missed by our automated tests and my manual testing: the issue with process only showed up at runtime (though our automated tests did catch it as well), not build or pack time, so we could be missing things that arn't tested and still pass CI.

@CraigMacomber
Copy link
Contributor Author

property-inspector and examples/data-objects/monaco (and a few other random packages I tested while modifying) are confirmed working with manual testing: they have been the most fragile, so I think this PR may be good to merge.

@CraigMacomber CraigMacomber changed the title Webpack 5 Webpack 5 for client mono-repo May 10, 2022
@CraigMacomber
Copy link
Contributor Author

awesome! there is one place we use webpack in the server. it's a test to ensure we can webpack local-server for demos and testing: fluid\server\routerlicious\packages\local-server\src\test\isomorphic.spec.ts

Doesn't need to be part of this PR, but would be good to get that updated as well. my guess is that is where the process dependency in many of our demos comes from

I specifically scoped server and docs code out of this change. I have updated the PR title accordingly. I'll take a look at that next.

@CraigMacomber CraigMacomber merged commit 9da2fc9 into microsoft:main May 10, 2022
anthony-murphy pushed a commit that referenced this pull request May 18, 2022
* update client mono-repo to webpack 5

* fix spaces example

* Fix spaces tests

* use process polyfill

* fix contact collection

* Use contenthash

* fix several tests by polyfilling process

* fix remaining tests

Co-authored-by: Craig Macomber <[email protected]>
curtisman added a commit that referenced this pull request Jun 2, 2022
PR #10186 upgraded to webpack 5, which caused the output of the bundle-size-test to be all 23 bytes with all export removed.

See #10186 (comment)

Adding `output.library` in the webpack config fixes the issue.
@CraigMacomber CraigMacomber deleted the webpack-5 branch May 17, 2024 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds: propertydds area: examples Changes that focus on our examples area: loader Loader related issues area: tests Tests to add, test infrastructure improvements, etc base: main PRs targeted against main branch dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants