module: significantly improve loader performance by BridgeAR · Pull Request #26970 · nodejs/node (original) (raw)

I guess the only concern here is cases like the following where you have say x.json:

require('./x'); // resolves x.json fs.writeFileSync('./x.js', 'module.exports = "new"'); require('./x'); // should resolve x.js

as surely the above case would get the JSON file again instead of the x.js contents?

No one will actually do that though, so this is fine, but in theory that is a break from semantics right?

Similarly if the relative resolution was running through package.json "main" or a directory index check, these would also be cached, with similar risks to the above right?

If this is done for package resolutions eg require('y') then we're basically just caching the resolver, which may not be intended. The restriction of the cache to the local directory sort of restricts the timeframe to make it feel like its correct, when in theory you could load a subpath of a package later. For example say require('pkg/index.js') loads require('dep') where require('pkg/index2.js') is then loaded much later might expect a different value for require('dep'). At least according to the current semantics that would be valid for require('dep') to be something else if the package.json changed. Again, no one would do that, but it's still not quite the same.

So for those sorts of reasons it worries me I must admit... it doesn't feel like this performance comes completely for free, although it certainly is close enough to free to feel like it could be worthwhile.