Using fs.writeFileSync in a serial style becomes especially costly when
we're writing a lot of files. In a recent profiling exercise I did on
Windows, nearly 80% of the time taken by Builder#_copyDirectory was spent
just closing the written files. By using the asynchronous fs.writeFile
function, we should be able to parallelize at least some of this work, and
await all the promises at the very end of copying the directory.
Unfortunately, this conversion triggered an error due to one of the
shortcomings of the Babel implementation of TypeScript:
SyntaxError: /tools/tool-env/profile.ts: Namespace not marked type-only declare. Non-declarative namespaces are only supported experimentally in Babel. To enable and review caveats see: https://babeljs.io/docs/en/babel-plugin-transform-typescript
278 | }
279 |
> 280 | export namespace Profile {
| ^
281 | export let enabled = !! process.env.METEOR_PROFILE;
282 |
283 | export function time<TResult>(bucket: string, f: () => TResult) {
at File.buildCodeFrameError (/Users/ben/meteor/dev_bundle/lib/node_modules/@babel/core/lib/transformation/file/file.js:261:12)
at transpileNamespace (/Users/ben/meteor/dev_bundle/lib/node_modules/@babel/plugin-transform-typescript/lib/namespace.js:25:25)
at PluginPass.TSModuleDeclaration (/Users/ben/meteor/dev_bundle/lib/node_modules/@babel/plugin-transform-typescript/lib/index.js:271:32)
at newFn (/Users/ben/meteor/dev_bundle/lib/node_modules/@babel/traverse/lib/visitors.js:193:21)
at NodePath._call (/Users/ben/meteor/dev_bundle/lib/node_modules/@babel/traverse/lib/path/context.js:53:20)
at NodePath.call (/Users/ben/meteor/dev_bundle/lib/node_modules/@babel/traverse/lib/path/context.js:40:17)
at NodePath.visit (/Users/ben/meteor/dev_bundle/lib/node_modules/@babel/traverse/lib/path/context.js:88:12)
at TraversalContext.visitQueue (/Users/ben/meteor/dev_bundle/lib/node_modules/@babel/traverse/lib/context.js:118:16)
at TraversalContext.visitMultiple (/Users/ben/meteor/dev_bundle/lib/node_modules/@babel/traverse/lib/context.js:85:17)
at TraversalContext.visit (/Users/ben/meteor/dev_bundle/lib/node_modules/@babel/traverse/lib/context.js:144:19)
Follow-up to af51b816, which fixed#8005 by copying symlinks to external
directories as directories rather than trying to preserve the symlinks.
Issue #10177 revealed a flaw in this strategy: the filter function that we
use to strip development npm packages always rejects external paths, even
if the original symlink was found in a valid production npm package, and
thus its contents should be included in the production bundle.
In the process of fixing this problem, I realized that the only important
part of af51b816 was this code:
// Update fileStatus to match the actual file rather than the
// symbolic link, thus forcing the file to be copied below.
fileStatus = optimisticLStatOrNull(externalPath);
and the code for manipulating thisAbsFrom and _currentRealRootDirectory
could be removed.
Because in-place rebuilds are disabled by default on Windows, we were
losing .meteor/local/build/programs/web.browser.legacy every time we
rebuilt .meteor/local/build as part of writeSiteArchive, as reported by
@lmachens in this comment: https://github.com/meteor/meteor/pull/9942#issuecomment-406656741
In-place rewriting of .meteor/local/build seems essential for the new
strategy of writing different targets at different times, though I have
attempted to limit the risk of overwriting open files on Windows by
continuing to build the individual target directories from scratch (that
is, by writing a fresh temporary directory and then renaming it over the
existing directory).
Though I don't have any specific ideas in this direction, it may be worth
noting: if we could find a way to make in-place builds safer on Windows
(as they are on Linux and Mac), Windows rebuilds would be significantly
faster than they are with the current strategy of rewriting everything
from scratch every time.
The bulk of this commit implements `builder.copyNodeModulesDirectory` to
allow more than one `node_modules` directory to be copied to the same
destination with as much safe symlinking as possible.
However, the crux of the fix for #9738 is the removal of the call to
`builder.generateFilename`, which deserves additional explanation.
If multiple directories are copied to the same output path by the builder,
in some cases it makes sense to ensure distinct directory names by adding
numeric suffixes to some of the directories.
In general, `builder.generateFilename` can get away with this renaming
only if the exact names of the directories are an implementation detail.
However, the code changed by this commit was altering the names of
`node_modules` directories whenever a package had both an `Npm.depends`
and a local `node_modules` directory.
Not only is it totally invalid to change the name of a `node_modules`
directory, but there is also no harm in copying the contents of multiple
`node_modules` directories into one final directory called `node_modules`.
Should fix#9738.
This was dangerous because source is often a path relative to the old
target file, whereas files.stat was interpreting source as a path relative
to process.cwd().
Fixes#9203.
The root of the problem was that the es5-ext npm package contains
directories called '#', e.g.
https://github.com/medikoo/es5-ext/tree/master/array/%23
These directory names were being sanitized to '' and thus ignored when
reserving paths in the Builder, which led to reservation conflicts later.
This commit fixes the problem in three different and independently
sufficient ways:
* Use files.mkdir_p instead of files.mkdir when creating parent
directories of written files.
* Replace illegal characters in sanitized paths with '_' instead of ''.
* Allow '#' in sanitized paths (only needs to be escaped in the shell, not
actually forbidden in paths).
writeSiteArchive already supported rebuilding in place (rather than
creating a new build directory and moving it into place; see comments
at top of builder.js), but it was not used and didn't work:
* run-app.js only passed previousBuilders to the bundler in the case
of a client-only refresh, in which case it also passed
hasCachedBundle, bypassing writeSiteArchive altogether.
* writeSiteArchive's use of previousBuilders seemingly didn't work,
because the site archive itself was never written in place, so
trying to write the targets "in place" into a brand-new build
directory didn't make sense (and threw an error).
With this change, previousBuilders are kept across all rebuilds
(not just client-only refreshes), which enables efficient, in-place
building (except on Windows, where in-place building has never been
supported), and writeSiteArchive is fixed to write the site archive
in place as well.
The only reason for the files.stat in atomicallyRewriteFile (called
by Builder#write), which was taking up time comparable to write
and rename (not as much as one of those but same order of
magnitude, in the hundreds of ms total)... was for the case where we
are trying to rename a file in a way that overwrites an existing
directory. We can avoid the stat by catching this unusual case when
rename throws an error.
