-
Notifications
You must be signed in to change notification settings - Fork 536
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
Conversation
Still needs a lot of testing and likely fixing, but at least the build passes, and one example (spaces) is known to be working. |
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. |
⯆ @fluid-example/bundle-size-tests: -1.93 MB
Baseline commit: d112cfb |
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.
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", |
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'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.
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.
Oh, I see now in a comment later:
We do however transitively depend on the
util
npm package (node_modules/util/util.js) which requiresprocess.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.
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 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.
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 added #10225.
"jest": "^26.6.3", | ||
"jest-junit": "^10.0.0", | ||
"jest-puppeteer": "^4.3.0", | ||
"process": "^0.11.10", |
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 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.
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.
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. |
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. |
* 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]>
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.
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.