feat(builtin): introduce a linker #1079
Conversation
alexeagle
force-pushed
the
linker
branch
2 times, most recently
from
419ee1c to
00059c6
Compare
internal/linker/link_node_modules.js
Outdated
| if (!!process.env['VERBOSE_LOGS']) console.error('[link_node_modules.js]', ...m); | ||
| } | ||
|
|
||
| function ls_r(d) { |
There was a problem hiding this comment.
risk of RangeError: Maximum call stack size exceeded with large trees.
There was a problem hiding this comment.
I removed the whole ls_r function, was really only needed while putting the code together
internal/linker/link_node_modules.js
Outdated
| for (const p of fs.readdirSync(d)) { | ||
| const f = path.join(d, p); | ||
| debug(' ', f); | ||
| if (fs.statSync(f).isDirectory()) { |
There was a problem hiding this comment.
could use the new readdir Dirent api here.
also potential for infinite recursion with symlinks that link to a parent. change to lstat or use dirent.isSymbolicLink
There was a problem hiding this comment.
ditto, let's just drop this code
internal/linker/link_node_modules.js
Outdated
|
|
||
| // Now add symlinks to each of our first-party packages so they appear under the node_modules tree | ||
| for (const m of Object.keys(modules)) { | ||
| if (fs.existsSync(m)) continue; |
There was a problem hiding this comment.
All fs operations could be done async and in parallel in this loop
There was a problem hiding this comment.
@soldair says he'll do some performance optimizations in a second PR once we've got tests in place that confirm this works
internal/linker/link_node_modules.js
Outdated
| } | ||
|
|
||
| function ls_r(d) { | ||
| for (const p of fs.readdirSync(d)) { |
There was a problem hiding this comment.
All fs operations could be done async and in parallel in this loop
internal/linker/link_node_modules.js
Outdated
|
|
||
| function symlink(target, path) { | ||
| debug(`symlink( ${path} -> ${target} )`); | ||
| fs.symlinkSync(target, path); |
There was a problem hiding this comment.
Would there be any perf gains to know if a file dosent need to be symlinked? Either at a file or directory level?
This would be for the situation where no files have changed but the process needs to restart
There was a problem hiding this comment.
that's what the logic does now, but we do it before calling the symlink helper.
I guess it would be more clear if the short-circuit just happens in the helper instead
|
Still to do here tomorrow:
|
alexeagle
force-pushed
the
linker
branch
2 times, most recently
from
7e3e23a to
7f5949d
Compare
alexeagle
force-pushed
the
linker
branch
3 times, most recently
from
fe02ff9 to
4c7a49e
Compare
This is a separate process we run right before node programs. Its job is to create symlinks so that the node_modules tree exists and has all the packages we might want to load at runtime. This will eventually replace the need for a custom module resolver, custom typescript path mappings, and the existing module_mappings.bzl support for TS/runtime mappings.
| // k: npm/node_modules/semver/LICENSE | ||
| // v: /path/to/external/npm/node_modules/semver/LICENSE | ||
| // calculate l = length(`/semver/LICENSE`) | ||
| if (k.startsWith(dir)) { |
There was a problem hiding this comment.
will this collide /a/applefactory with /a/apple?
missing k.startsWith(dir+'/') || k === dir here i think
There was a problem hiding this comment.
agreed, will add a test for this in a followup
This is a separate process we run right before node programs.
Its job is to create symlinks so that the node_modules tree exists and
has all the packages we might want to load at runtime.
This will eventually replace the need for a custom module resolver,
custom typescript path mappings, and the existing module_mappings.bzl
support for TS/runtime mappings.