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

fix: bundle blockly test images in block-test plugin #2024

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alicialics
Copy link
Contributor

@alicialics alicialics commented Oct 19, 2023

The basics

This PR will address issues like bundling assets (specifically tested with png files in @blockly/block-test) in webpack UMD bundles, as well as potential future plugin ESM builds.

It makes use of the following:

The details

Resolves

Fixes #503
Fixes #1145

This bundles needed static media files from https://github.com/google/blockly/tree/develop/tests/media into @blockly/block-test package

@maribethb and I discussed in detail (see #1969) on what potential approaches that could enable plugins to bundle assets in their builds. The universal way is inline assets using dataUrls, which is not that interesting in itself and may negatively impact performance.

The approach in this PR requires more tooling support depending on how you ingest the package in your app however the upside is that Webpack supports this natively and there is a minimal change to @blockly/dev-scripts.

Here are the details:
Webpack will include the png files as <hash>.png in /dist in @blockly/block-test

@alicialics ➜ /workspaces/blockly-samples/plugins/block-test (import-block-test-assets) $ ls dist/
0ed404116fc61b34ab03.png  e646c1dc1540e78440bb.png  index.js.LICENSE.txt
4daf5c075a6ffed5369f.png  index.js                  index.js.map

package.json module field still points to /src with code new URL('../media/30px.png', import.meta.url)
bundlers like Webpack will use the module field to build any code that references this plugin via the module field.
So code like @blockly/dev-tools or examples/blockly-react will also include png files as <hash>.png in their dist/

I also briefly tested removing module field from @blockly/block-test's package.json and it seems that @blockly/dev-tools will still build fine and include all the assets.

@alicialics ➜ .../blockly-samples/plugins/dev-tools/dist (import-block-test-assets) $ ls
0ed404116fc61b34ab03.png  e646c1dc1540e78440bb.png  index.js.LICENSE.txt
4daf5c075a6ffed5369f.png  index.js                  index.js.map

npm start in @blockly/block-test will also work now

For plain html files, import.meta will work when you do a script tag with type module, ie:
<script type="module" src="./path/to/@blockly/block-test/dist/index.esm.js">

However we are not building esm format at the moment (see #1877 and #1997) so this is not possible today.

Lastly if you just do a plain script tag in a html file <script src="./path/to/@blockly/dev-tools/dist/index.js"> (eg dev-tools imports block-test as mentioned above) we rely on publicPath: "auto" configuration in webpack to use document.currentScript.src to figure out the correct URL path to the asset in question, such as in advanced playground

The relevant change in the UMD output is:

93383,93402c93383
<         (() => {
<           var l;
<           __webpack_require__.g.importScripts &&
<             (l = __webpack_require__.g.location + "");
<           var n = __webpack_require__.g.document;
<           if (!l && n && (n.currentScript && (l = n.currentScript.src), !l)) {
<             var e = n.getElementsByTagName("script");
<             if (e.length)
<               for (var t = e.length - 1; t > -1 && !l; ) l = e[t--].src;
<           }
<           if (!l)
<             throw new Error(
<               "Automatic publicPath is not supported in this browser",
<             );
<           (l = l
<             .replace(/#.*$/, "")
<             .replace(/\?.*$/, "")
<             .replace(/\/[^\/]+$/, "/")),
<             (__webpack_require__.p = l);
<         })(),
---
>         (__webpack_require__.p = "/dist/"),

Proposed Changes

See above for details

Reason for Changes

Fixes #503
Fixes #1145 (this should provide a way to load any audio assets but I have not tested)

Test Coverage

npm run start in @blockly/block-test

Documentation

N/A

Additional Information

Preferably requires more testing from the blockly team to ensure @blockly/dev-tools still works as expected since it imports @blockly/block-test

cc @cpcallen

@alicialics alicialics requested a review from a team as a code owner October 19, 2023 19:30
@alicialics alicialics requested review from maribethb and removed request for a team October 19, 2023 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants