feat: add webpack asset loaders

[alicialics]

The basics

• I validated my changes

The details

[for ghc-osd 2023 event]

This PR loads common image and audio assets inline so plugin developers can easily incorporate their own assets.
An example is included for workspace-backpack plugin.

Resolves

Fixes #1145

Proposed Changes

• Adds blockly-env.d.ts to create-package templates
• Add default blockly-env.d.ts to @blockly/dev-scripts + types field in package.json
• Fixes blockly-env.d.ts references in eslint
• Add asset/inline module loader in webpack.config.js blockly-scripts
• Add example for workspace-backpack

Reason for Changes

Test Coverage

ran npm run deploy:prepare:plugins
build, lint, started devserver for workspace-backpack and verified svg was loaded correctly

Documentation

TBD, please let me know.

Additional Information

The reason I chose asset/inline over asset/resource is that as a library if some asset file is produced in the ./dist folder your host webapp need to pick it up and serve it themselves. I don't think this approach would work. So the images definitely needs to be inlined. I am less familiar with audio files but I assume they work similarly.

Edit: I just tested asset/resource and it seems to work as well when I'm testing (npm start) workspace-backpack.. I believe it works because we use webpack to build a devServer that serves the svg at the right place. However if the plugin is used in a different webapp (eg github pages) then unless webpack or another build system does some module magic and put the assets in a servable bundle I don't think having assets as a seperate file will work [ie have to inline them]. [from my testing github pages did not work as expected because test_bundle.js was looking for its svg files but they are not found]. If we can confirm this somehow works then it's best to use 'type': 'asset' to let webpack auto determine whether the resource should be inlined or in a separate file.

I am not a huge fan of using webpack to bundle libraries. I prefer this is moved to tsup (using esbuild) or rollup.

[conventional-commit-lint-gcf]

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

[BeksOmega]

@maribethb This isn't a change we planned on including as part of GHC, but looks reasonable to me! I'm going to assign this to you since it may need discussion. Happy to take some of your PRs in return!

[maribethb]

Hi, thank you for opening this PR and looking into this issue.

Unfortunately, I don't think it meets our goals here. For audio files, we wouldn't want to inline the entire file. I understand how that makes sense from the perspective of users of the plugin not having to host the media file themselves, but it would make the file size a lot bigger for an audio file.

I think instead, we need to configure webpack to simply copy over the asset in its entirety to the dist directory so that the audio assets are available to users of the plugin. Making the audio file available in their own webserver would be the responsibility of the plugin user. Then any plugins that use audio assets (n.b., none, as of today) should make the URL at which to look for the asset configurable by the user. That would mean someone could configure their own build process to copy the file from node_modules/@blockly/workspace-backpack/dist/mySound.mp3 to be served from public/assets/mySound.mp3 and then pass in public/assets/ as the location for which the plugin should look for mySound.mp3. This is basically how Blockly core handles the same issue.

We may be able to set the default location of the file to use the new import.meta.url feature but I think we should still let folks pass in a URL for anyone who wants to configure their server a certain way. I am not entirely sure what the code would look like for this to work, because I don't really understand what webpack is doing to these URLs under the hood.

---

For svg files, inlining them does make sense. The files are already small and inlining them lets us avoid making users figure out how to host images. We were already inlining them by providing the dataUri for each SVG. The work done here makes it possible to inline the images automatically instead of having to provide that dataUri manually. However, it also requires adding the additional scaffolding as you've seen (e.g. configuring TypeScript types for the new file types, changing the eslint settings). Because we have such a small number of images, I don't think this extra scaffolding is worth it, and we should just continue to "manually" inline the images. If folks disagree, I'd be happy to take another look, but I think the way the new types are linked in would need another look in that case (to avoid using the triple slash directive)

---

I hope this makes sense. Let me know how you'd like to proceed. I think we should probably close this pull request, but if you'd like to have a look at just configuring webpack to copy over mp3/other audio assets and want to use this same branch/PR that would be fine too. Let me know if you have any questions! Sorry we can't accept this PR in this form, but we do appreciate your work on this!

[alicialics]

Unfortunately, I don't think it meets our goals here. For audio files, we wouldn't want to inline the entire file. I understand how that makes sense from the perspective of users of the plugin not having to host the media file themselves, but it would make the file size a lot bigger for an audio file.

I agree with this assessment. In addition, I'd like to make the following observations: no plugins uses audio files today, and plugin developers are not forced to inline their audio files using this approach. They can always ask the users of their plugin to copy certain assets to a servable directory for the plugin to use. Also my understanding is that most sound effects are short and small(like 3kb-7kb) while some plugin's js payloads are 15kb+

I think instead, we need to configure webpack to simply copy over the asset in its entirety to the dist directory so that the audio assets are available to users of the plugin.

This is easy, simply use type: 'asset/resource' or type: 'asset'. However this generates a file with some kind of hash, it should probably be a stable name like /dist/asset/foo.mp3 but this shouldn't be that hard to do.

Making the audio file available in their own webserver would be the responsibility of the plugin user. Then any plugins that use audio assets (n.b., none, as of today) should make the URL at which to look for the asset configurable by the user. That would mean someone could configure their own build process to copy the file from node_modules/@blockly/workspace-backpack/dist/mySound.mp3 to be served from public/assets/mySound.mp3 and then pass in public/assets/ as the location for which the plugin should look for mySound.mp3. This is basically how Blockly core handles the same issue.

I think it's doable but it may open a can of worms. Mainly, how would the user know which plugins needs this extra step? I would not expect it if 90% of the plugins worked fine but there was one or two that required me to copy a few files to different directories (ideally each plugin should have its own asset directory). Also we need to potentially consider the situation where we allow plugins that depends on other plugins..

We may be able to set the default location of the file to use the new import.meta.url feature but I think we should still let folks pass in a URL for anyone who wants to configure their server a certain way. I am not entirely sure what the code would look like for this to work, because I don't really understand what webpack is doing to these URLs under the hood.

I'd be happy to test this but currently plugins only have examples of svg files. But I guess I can just use svg files for testing if the blockly core team decide to try this approach.

For svg files, inlining them does make sense. The files are already small and inlining them lets us avoid making users figure out how to host images. We were already inlining them by providing the dataUri for each SVG. The work done here makes it possible to inline the images automatically instead of having to provide that dataUri manually. However, it also requires adding the additional scaffolding as you've seen (e.g. configuring TypeScript types for the new file types, changing the eslint settings). Because we have such a small number of images, I don't think this extra scaffolding is worth it, and we should just continue to "manually" inline the images. If folks disagree, I'd be happy to take another look, but I think the way the new types are linked in would need another look in that case (to avoid using the triple slash directive)

additional scaffolding is fairly common, and since we are adding it to templates my personal opinion it won't effect new plugin developers (they may have issues if they are working on an existing plugin and bumping dev-scripts version however I assume they are probably just using dataUri and this won't affect them as well). This is also standard practice among frameworks like Next, CRA: https://github.com/vercel/next.js/blob/canary/packages/create-next-app/templates/app-tw/ts/next-env.d.ts

I hope this makes sense. Let me know how you'd like to proceed.

Ultimately this is the blockly core team member's call but I am okay with whatever the team decides.

[maribethb]

Requiring developers to host the audio assets themselves is certainly tricky, but I don't know of a way around that at the moment. Any plugin that wants to add audio assets would have to clearly document this requirement in the README. We do expect users of plugins to read the documentation for that plugin, so I believe this is a surmountable problem.

The piece we're missing right now is just bundling that asset with the published plugin. We may want to use CopyWebpackPlugin to simply copy the files instead of using an asset loader.

Regarding the inline SVGs and the additional TypeScript scaffolding: I don't like the magic of the triple-slash directives. It is confusing to me and it's not a clear way of following the dependency chain. I think we still don't need this feature right now, but if we did, we could upgrade to TS 5 and use this new flag. That would remove the need for magic module names; any plugin importing a fileName.svg file could also add a matching fileName.d.svg.ts file to satisfy the compiler.

For now, I'll close this PR. If you want to work on the audio bundling issue, you can either open a new PR or re-open this one. Thank you for the back and forth :)

[alicialics]

This is basically how Blockly core handles the same issue.

Can you share me how blockly asks developers to handle its media assets? I don't see it in the docs.

I find it difficult to find examples how one would use blockly and blockly-plugins as a developer.

That would remove the need for magic module names; any plugin importing a fileName.svg file could also add a matching fileName.d.svg.ts file to satisfy the compiler.

In my experience, developers would generally prefer a single d.ts file that encapuslates all svg files rather than adding a ts file for each svg file they are exporting.

The piece we're missing right now is just bundling that asset with the published plugin. We may want to use CopyWebpackPlugin to simply copy the files instead of using an asset loader.

This requires setting up a separate configuration file like blockly.config.ts and let people modify/inject webpack configuration. I am not convinced that dev-scripts should even be using webpack to create a library bundle in the first place. There are much better options like esbuild and rollup, and once blockly.config.ts is introduced with webpack configuration its much harder to go back.

If you want to work on the audio bundling issue, you can either open a new PR or re-open this one.

I'd love to reach some kind of agreement on how best to approach this before attempting another PR. There are related issues such as on blockly and blockly plugins should bundle eg #1877 and google/blockly#7449 cc @cpcallen

[BeksOmega]

Can you share me how blockly asks developers to handle its media assets? I don't see it in the docs.

By default we point to https://blockly-demo.appspot.com/static/media/ for our media assets, but external devs can configure it to point where ever. It is one of our inject options =)

This requires setting up a separate configuration file like blockly.config.ts and let people modify/inject webpack configuration.

Could you explain a bit more about why this is required? I don't know much about the ConfigureWebpackPlugin but I was assuming you could have it copy based on a regexp! E.g. copy all mp4 files.

There are much better options like esbuild and rollup

Would you be interested in writing up an issue about this? I think it's unlikely that we change the build system because there's a lot of infrastructure around it, and changing it would be a significant investment. But it would be interesting to hear your perspective on the pros and cons, since it seems like you've worked on other similar open source projects before!

I'd love to reach some kind of agreement on how best to approach this before attempting another PR.

Makes sense! Happy to keep chatting about it more =)