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

chore(build): use inline sources in sourcemaps; don't package sources separately #6362

Merged
merged 2 commits into from
Aug 22, 2022

Conversation

cpcallen
Copy link
Contributor

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm run lint

The details

Resolves

tsc compile failures for developers whose build pipeline is misconfigured in a way that causes it to ingest .ts source files from node_modules/blockly/core/—somehow without also ingesting core/any_aliases.ts—resulting in compile errors due to AnyDuringMigration. (See e.g. item #2 of #6358.)

Proposed Changes

Don't package the source .ts files; instead, include sources inline in the .js.map sourcemaps.

Behaviour Before Change

.ts files are copied from core/, blocks/ and generators/ to dist/.

Sourcemap (.js.map) files contain a two-byte sourceRoot property.

Behaviour After Change

.ts files are not packaged.

Sourcemap files contain a large sourcesContent property (~2MB for `blockly_compressed.js.map), containing all of the original source.

Reason for Changes

@btw17 points out that it is [generally not advised to package .ts sources along with compiled code in NPM packages]https://stackoverflow.com/a/57534667/4969945), and empirically it does appear to be causing problem for some of our developers.

Test Coverage

Manually verified that sourcemaps with inline sources work correctly in compiled and uncompiled mode.

Additional Information

I sent email to @samelhusseini to get clarity about why sources were originally included in the Blockly NPM package. I think it worth waiting for a response before merging this PR.

Since the sources are now inline in the sourcemaps, they no longer
need to be package separately.
Copy link
Contributor

@maribethb maribethb left a comment

Choose a reason for hiding this comment

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

LGTM assuming we have sam's blessing. were you able to test the resulting sourcemaps work as expected in a plugin?

@cpcallen
Copy link
Contributor Author

LGTM assuming we have sam's blessing.

I will assign to Rachel to submit when we have a reply (or she decides we don't need one).

were you able to test the resulting sourcemaps work as expected in a plugin?

That is one configuration I did not test, but I did test loading in compiled mode which uses the same .js.map files so I'm 95% sure this will work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: build process PR: chore General chores (dependencies, typos, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants