-
Notifications
You must be signed in to change notification settings - Fork 2k
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: strictly verify imports and disable nodejs polyfilling #22630
Conversation
I noticed that too when working on the REST Proxy 2.0. The library can run also in Node.js and there it supports passing a file name as a file argument. The file will be opened, read and uploaded as a media file. https://github.com/Automattic/wpcom.js/blob/master/lib/site.media.js#L23 |
webpack.config.js
Outdated
new webpack.DefinePlugin( { | ||
'process.env.NODE_ENV': JSON.stringify( bundleEnv ), | ||
process: { env: { NODE_ENV: JSON.stringify( bundleEnv ) } }, |
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.
Just a not that webpack
advises against this change
https://webpack.js.org/plugins/define-plugin/#feature-flags
[this way it has changed] will overwrite the process object which can break compatibility with some modules that expect other values on the process object to be defined.
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.
not also that this directly undoes the cleanup from #15691
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.
Initially I was going to reply that this is intentional but clearly not ideal. It makes sense to only overwrite a specific property as a constant when polyfilling process
, but what about when all of process should be undefined
because we've disabled the polyfill?
I'm revisiting and trying to make it so that none of our code depends on any non-explicitly included polyfills...which I didn't realize is harder than I thought. node: false
doesn't seem to be working like I thought it would!
Things like import path from 'path'
and import qs from 'querystring'
still work!
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've re-cleaned it up. Thank you for pointing this out
6674409
to
6222423
Compare
2692463
to
47d2ae6
Compare
in its current form, this pr drops build by 5 kBgz. Probably from changing
|
This could be a separate PR. The |
separate pr for the lru swap: #22714 |
update The good news is that there are eslint plugins that can accomplish exactly what we want. By setting
|
3ce867c
to
52aeba2
Compare
70c6843
to
fe1ac77
Compare
@blowery w.r.t
edit: maybe just do a DefinePlugin for |
d4036f9
to
41e909b
Compare
1. node libs 2. libs that are not in package.json
41e909b
to
181b6db
Compare
9616b0c
to
035ef55
Compare
final update: included
deferred:
ICFY: http://iscalypsofastyet.com/branch?branch=update/webpack/node-false
to test
|
because calypso.live is breaking on this branch name, I've duped this branch on #22925 just for e2e and canary tests |
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.
Very impressive cleanup indeed! Thanks for working on this.
@@ -101,7 +104,8 @@ export default class Step extends Component { | |||
this.quitIfInvalidRoute( nextProps, nextContext ); | |||
this.skipIfInvalidContext( nextProps, nextContext ); | |||
this.scrollContainer.removeEventListener( 'scroll', this.onScrollOrResize ); | |||
this.scrollContainer = query( nextProps.scrollContainer )[ 0 ] || global.window; | |||
this.scrollContainer = | |||
query( nextProps.scrollContainer )[ 0 ] || ( typeof window !== 'undefined' && window ); |
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.
Is the typeof window
check necessary? If window
is undefined, then the app will crash on the very next line anyway (addEventListener
).
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.
it may not be necessary. I know sometimes it is to prevent tests from failing.
seems like the typeof
check isn't covering enough in this case.
Since tests are all passing, I can remove this check and someone can add it back in if the future if we ever need it
@@ -3,7 +3,7 @@ | |||
/** | |||
* External dependencies | |||
*/ | |||
import assert from 'assert'; | |||
import assert from 'assert'; // eslint-disable-line import/no-nodejs-modules |
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.
It would be awesome and quite simple to migrate to import { assert } from 'chai'
here. But that's for another PR. A codemod would help a lot with the migration for sure.
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.
a small multi-file find/replace would probably work as well.
I'll leave that for a small follow-up pr
// file. Webpack uses the same module on the client side, too, which | ||
// makes for a nice consistency. | ||
import { EventEmitter } from 'events/'; | ||
import { EventEmitter } from 'events'; |
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.
Did you verify somehow that the domain
property on EventEmitter
-decorated objects is no longer a threat? The Site
class mentioned in the comment is gone, but there might be other usages that are still live.
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'll be frank: I don't understand the comment or how it could make sense.
webpack
never bundled the actual "node module". It always used a web polyfill. In this specific case it was Gozala/events which is published to npm as events
. Therefore I don't think the /
would change anything...
Also, if you look at the source of events
, you won't find domain
anywhere.
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.
discussed on slack. I think the comment was largely historical based on where this file used to live in the pre-oss days
client/my-sites/preview/main.jsx
Outdated
@@ -48,11 +48,15 @@ class PreviewMain extends React.Component { | |||
debouncedUpdateLayout = debounce( this.updateLayout, 50 ); | |||
|
|||
componentDidMount() { | |||
global.window && global.window.addEventListener( 'resize', this.debouncedUpdateLayout ); | |||
if ( typeof window !== 'undefined' ) { | |||
window && window.addEventListener( 'resize', this.debouncedUpdateLayout ); |
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.
Checking the window
truthiness for a second time is not necessary.
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'll remove the second check
As discussed in an issue earlier, the best way to make sure we keep our bundled code size lean is to make ensure we stop it from growing. This PR focuses on that goal by modifying how we interact with the nodejs modules.
nodejs has a standard library that does not exist in browsers. this dichotomy is confusing to develop in, and in order to alleviate some of the resulting frustration webpack automatically polyfills many of these libs. The problem: to someone that doesn't know, these libs appear as if they are just freely available but actually they don't come free, and can easily cause bloat. We should include them with the same scrutiny that we use when including any other npm dependency.
In order to fix this going forward there were two changes I needed to make:
This could have saved us in a few cases like:
notes as i've been going along:
jitm
data-layer uses the variableprocess
. I confirmed locally that shadowing the variable still works even though we are overwriting process withDefinePlugin
.crypto
. I don't know why, but webpack doesn't allow for that explicitly, which is why I needed set global to{}
with DefinePluginlru-cache
, which isn't even a declared dependency in package.json. The only reason Calypso is compiling is because it is a dependency of a dependency. It was written for node and node for the browser. I'll replace it withlru
.To test
add these snippets to any file in client and make sure it crashes the build: