Unify importing of node fs and path modules #15587
Conversation
sbc100
force-pushed
the
unify_node_imports
branch
2 times, most recently
from
d0521a8 to
cf23a96
Compare
tests/test_other.py
Outdated
| self.assertLess(changed, normal) | ||
| # Check an absolute code size as well, with some slack. | ||
| self.assertLess(abs(changed - 5001), 150) | ||
| self.assertLess(abs(changed - 5174), 150) |
There was a problem hiding this comment.
The rebase here is actually going from 5150 to 5174 so 24 bytes difference from main
sbc100
force-pushed
the
unify_node_imports
branch
from
cf23a96 to
3f07a44
Compare
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
Maybe with that comment the code size regression goes away? If not, why is it getting worse? I'd expect closure to inline those calls and end up with similar size as before, unless the issue is that we include both of the modules at each site which we didn't before.
| requireNodeFS(); | ||
| filename = nodePath['normalize'](filename); | ||
| nodeFS['readFile'](filename, function(err, data) { | ||
| fs.readFile(filename, function(err, data) { |
There was a problem hiding this comment.
Are you sure the quoting is not needed here? Is closure aware of the Node.js FS API?
src/node_shell_read.js
Outdated
| if (!fs) fs = require('fs'); | ||
| if (!nodePath) nodePath = require('path'); |
There was a problem hiding this comment.
| if (!fs) fs = require('fs'); | |
| if (!nodePath) nodePath = require('path'); | |
| // We always initialize both of these together, so we can use | |
| // either one as the indicator for them not being initialized. | |
| if (!fs) { | |
| fs = require('fs'); | |
| nodePath = require('path'); | |
| } |
This is I think smaller?
There was a problem hiding this comment.
I still see a 19 byte regression in this test (although we have 150 byte tollerance here).
Its due to the function itself being part of the output:
E = function() { fs || (fs = require("fs"), D = require("path")) },
Since this is tiny and will not be present for folks that set ENVIRONMENT=web anyway I think its fine.
There was a problem hiding this comment.
None of actual code size tests regress even one byte.
There was a problem hiding this comment.
Ah right, good point that it just affects node-enabled builds.
We were importing these modules in different places and under different names. I went with `fs` for the name of the `fs` import since that is what `library_nodefs.js` and it is by far the heaviest user of this module.
sbc100
force-pushed
the
unify_node_imports
branch
from
3f07a44 to
c39723d
Compare
We were importing these modules in different places and under different names. I went with `fs` for the name of the `fs` import since that is what `library_nodefs.js` and it is by far the heaviest user of this module.
We were importing these modules in different places and
under different names. I went with
fsfor the nameof the
fsimport since that is whatlibrary_nodefs.jsand it is by far the heaviest user of this module.