Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(build): Remove closure-make-deps and closure-calculate-chunks #7473

Merged
merged 4 commits into from
Sep 12, 2023

Conversation

cpcallen
Copy link
Contributor

@cpcallen cpcallen commented Sep 8, 2023

The basics

The details

Resolves

Fixes #7469

Proposed Changes

  • Rewrite the getChunkOptions function to not use closure-calculate-chunks, but instead just chunk the input files (more or less) by subdirectory: first chunk is core/, second is blocks/, etc.

    This does make a material change to blockly_compressed.js, because we end up feeding several empty modules that contain only typescript interface declarations and which tsc compiles to export {}; in the input to Closure Compiler
    (closure-calculate-chunks is smart enough to notice that no other module depends on these), which results in ~1.7KiB of
    superflous var module$build$src$core$interfaces$i_ast_node_location_svg={}; declarations. This can be avoided by filtering such empty modules out, but that has been left for a future commit.

    This adds the glob NPM package as a dev dependency, but gulp and several other existing dev dependencies already depend on it.

    Build time is sped up by about a factor of 3x, due to being able to skip the buildDeps step that was really slow:

    $ time npm run build
    # Before:
    real    0m24.410s
    user    0m16.010s
    sys     0m1.140s
    # After
    real    0m8.397s
    user    0m11.976s
    sys     0m0.694s
    
  • Remove the buildDeps task.

  • Remove the $build$src infix from munged paths.

    Closure Compiler renames module globals so that they do not clash when multiple modules are bundled together. It does so by adding a $$module$build$src$path$to$module suffix (with the module object itself being named simply $module$build$src$path$to$module).

    By changing the gulp.src base option to be build/src/ instead of ./ (referring to the repository root), Closure Compiler obligingly shortens all of these munged names by removing the $build$src infix, reducing the size of the compressed chunks by about 10%; blockly_compressed.js goes from 900595 to 816667 bytes.

  • Compute module object munged name from entrypoint:

    • Add function modulePath to compute the munged name of the entrypoint module from the entrypoint module filename, and use this instead of hard-coded chunk.exports paths.
    • Be more careful about when we are using posix vs. OS-specific paths, especially TSC_OUTPUT_PATH which is regularly passed to gulp.src, which is documented as taking only posix paths.
    • Rename one existing variable modulePath -> entryPath to try to avoid confusion with the new modulePath function.

Reason for Changes

Fewer dependencies, faster builds, simpler build config!

Test Coverage

npm test passes; playground loads in compressed and uncompressed mode.

Rewrite the getChunkOptions function to not use
closure-calculate-chunks, but instead just chunk the input files
(more or less) by subdirectory: first chunk is core/, second is
blocks/, etc.

This does make a material change to blockly_compressed.js,
because we end up feeding several empty modules that contain
only typescript interface declarations and which tsc
compiles to "export {};" in the input to Closure Compiler
(closure-calculate-chunks is smart enough to notice that
no other module depends on these), which results in ~1.7KiB of
superflous

    var module$build$src$core$interfaces$i_ast_node_location_svg={};

declarations.  This can be avoided by filtering such empty modules
out but that has been left for a future commit.

This adds the glob NPM package as a dev dependency, but gulp
and several other existing dev dependencies already depend on
it.

Build time is sped up by about a factor of 3x, due to removal
of the buildDeps step that was really slow:

$ time npm run build

before:
real    0m24.410s
user    0m16.010s
sys     0m1.140s

after:
real    0m8.397s
user    0m11.976s
sys     0m0.694s
Closure Compiler renames module globals so that they do not
clash when multiple modules are bundled together.  It does so
by adding a "$$module$build$src$path$to$module" suffix (with
the module object istelf being named simply
"$module$build$src$path$to$module").

By changing the gulp.src base option to be build/src/ instead
of ./ (referring to the repostiory root), Closure Compiler
obligingly shortens all of these munged named by removing the
"$build$src" infix, reducing the size of the compressed chunks
by about 10%; blockly_compressed.js goes from 900595 to 816667
bytes.
- Add modulePath to compute the munged name of the entrypoint
  module from the entrypoint module filename, and use this
  instead of hard-coded chunk.exports paths.
- Be more careful about when we are using poxix vs. OS-specific
  paths, especially TSC_OUTPUT_PATH which is regularly passed
  to gulp.src, which is documented as taking only posix paths.
- Rename one existing variable modulePath -> entryPath to try
  to avoid confusion with new modulePath function.
@rachel-fenichel
Copy link
Collaborator

I'm excited about this one!

In the past we've had a few developers who exclusively use Windows report issues with our build system. Please tag in one or two of them to check that this still works on Windows.

Copy link
Collaborator

@rachel-fenichel rachel-fenichel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this PR.

In addition to finding someone to test on Windows, it's worth packaging up a beta and testing it with examples and plugins to make sure nothing was magically depending on a detail of the build system.

scripts/gulpfiles/build_tasks.js Show resolved Hide resolved
@cpcallen
Copy link
Contributor Author

I love this PR.

Thanks!

In addition to finding someone to test on Windows, it's worth packaging up a beta and testing it with examples and plugins to make sure nothing was magically depending on a detail of the build system.

I attempted to test this myself on Windows, but I don't know a lot about how Windows devs typically set up their machines.

@cpcallen cpcallen merged commit e7030a1 into google:develop Sep 12, 2023
11 checks passed
@cpcallen
Copy link
Contributor Author

@yamadayutaka: You have been our main Windows developer when it comes build system testing. Can you pull this change (now merged to develop and verify that npm run clean ; npm test succeeds on Windows?

Also, I now have access to a Windows machine but am not sure how to set it up in a way that best represents how developers like you work. An explanation of how you have things set up (what did you install, and from where? Are you using WSL + bash or just Command Prompt? etc. etc.) would be very useful.

@yamadayutaka
Copy link
Contributor

yamadayutaka commented Sep 13, 2023

Hi @cpcallen, I tried running npm test in my environment and got the following error.

=======================================
== renamings
'tests' is not recognized as an internal or external command,
operable program or batch file.
Command failed: tests/migration/validate-renamings.mjs
FAILED: renamings

The validate-renamings.mjs is executed by shebang, but it does not seem to work on Windows.

If you change the renamings function as follows, it works fine on Windows.

function renamings() {
-  return runTestCommand('renamings', 'tests/migration/validate-renamings.mjs');
+  return runTestCommand('renamings', 'node tests/migration/validate-renamings.mjs');
}

Also, I now have access to a Windows machine but am not sure how to set it up in a way that best represents how developers like you work. An explanation of how you have things set up (what did you install, and from where? Are you using WSL + bash or just Command Prompt? etc. etc.) would be very useful.

I think it would be better to be able to build with PowerShell Propmpt instead of using WSL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove closure-make-deps and closure-calculate-chunks
3 participants