Support watching of config files #460
Conversation
Codecov Report
@@ Coverage Diff @@
## master #460 +/- ##
==========================================
- Coverage 81.08% 78.77% -2.32%
==========================================
Files 6 6
Lines 185 179 -6
Branches 49 46 -3
==========================================
- Hits 150 141 -9
Misses 18 18
- Partials 17 20 +3
Continue to review full report at Codecov.
|
src/utils/read.js
Outdated
| @@ -7,16 +7,10 @@ const fs = require("fs"); | |||
| * var read = require('./helpers/fsExists')({}); | |||
| * read('.babelrc'); // file contents... | |||
| */ | |||
| module.exports = function(cache) { | |||
| cache = cache || {}; | |||
There was a problem hiding this comment.
Removing this cache makes babel-loader got to the file-system to load the config for every file that it compiles, which seems not to be a good idea and would be a major performance hit.
I see that in watch you might want to rebuild with the new config, but I honestly rather go for a performant loader that works for 99% of the people (who can live with restarting watch when the config changes) than a slow but more correct one.
Or can you explain further on this change? Maybe I'm missing something.
There was a problem hiding this comment.
@danez now cache provide old config file when changed, i known what it is not best, I'll try to find a solution, also related to resolve-rc.js. While it's possible that caching such a place is just the wrong solution.
There was a problem hiding this comment.
@danez main problems not in restarting watch when the config changes, main problems in watch graph (absence config file in dependencies), it is break loader caches before babel-loader
danez
changed the title
|
@danez Sad but I could not find a solution. I don't know what can we do with this, but I feel this is a strong problem. It seems that caching a configuration is a bad idea. |
|
You can use the filesystem provided via |
|
@sokra That is awesome. I was just searching on how to find out what change is triggering the loader run, so that we could invalidate the cache, but that is even easier. |
|
@danez friendly ping |
src/utils/exists.js
Outdated
| /** | ||
| * Check if file exists and cache the result | ||
| * return the result in cache | ||
| * | ||
| * @example | ||
| * var exists = require('./helpers/fsExists')({}); | ||
| * exists('.babelrc'); // false | ||
| * var exists = require(require('fs'), './helpers/fsExists')({}); |
| cache[filename] || | ||
| (fs.existsSync(filename) && fs.statSync(filename).isFile()); | ||
| try { | ||
| exists = fileSystem.statSync(filename).isFile(); |
There was a problem hiding this comment.
Can we do fs.existsSync(filename) here instead of exception catching. Try catch will make v8 deoptimize this function.
There was a problem hiding this comment.
@danez webpack fs doesn't fs.existsSync, because exists is deprecated
There was a problem hiding this comment.
I see, maybe webpack would be open to add existsSync as it was undeprecated in 7.0.
There was a problem hiding this comment.
@danez hm, your are right, in theory we can create issue and add comment in code, and when issue was resolved change code?
There was a problem hiding this comment.
@danez btw, webpack have many places where use statSync and this does not cause any problems with performance, i think this can be left
src/utils/read.js
Outdated
| /** | ||
| * Read the file and cache the result | ||
| * return the result in cache | ||
| * | ||
| * @example | ||
| * var read = require('./helpers/fsExists')({}); | ||
| * read('.babelrc'); // file contents... | ||
| * var read = require(require('fs'), './helpers/fsExists')({}); |
src/utils/read.js
Outdated
| return cache[filename]; | ||
| }; | ||
| // Webpack `fs` return Buffer | ||
| return fileSystem.readFileSync(filename, "utf8").toString(); |
There was a problem hiding this comment.
As the encoding is specified, we do not need to do .toString() here.
There was a problem hiding this comment.
@danez fileSystem.readFileSync in webpack fs doesn't support second argument and always return Buffer, i try to investigate this
There was a problem hiding this comment.
@danez seems some fs functions don't support more than one argument https://github.com/webpack/enhanced-resolve/blob/master/lib/CachedInputFileSystem.js#L197.
There was a problem hiding this comment.
Practically
fileSystem.readFileSync(filename).toString("utf8"); === fileSystem.readFileSync(filename, "utf8");.
https://github.com/nodejs/node/blob/master/lib/fs.js#L602
|
@danez friendly ping |
|
Thank you very much this looks very good. Nice that we found this cool approach with |
Please Read the CONTRIBUTING Guidelines
In particular the portion on Commit Message Formatting
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
Loader don't watch configuration files. Also don't adding configuration breaks other loader before
babel-loader. And provide invalidate config after change.What is the new behavior?
Watch on config files.
Does this PR introduce a breaking change?
If this PR contains a breaking change, please describe the following...
Other information:
Ref: #456 (comment)
Ref: #456 (comment)