repack icon indicating copy to clipboard operation
repack copied to clipboard

More control over file path and URL for remote assets

Open adammruk opened this issue 1 year ago β€’ 14 comments

Hi team!

I'm working with remote assets, and this feature looks very promising. However, there's one feature that I'm missing: the ability to specify the URL to the file.

Describe the Feature

This feature would allow for more control over the URL of remote assets. Currently, the folder structure in the build folder generated for remote assets and the URL represents the folder structure in the repository. I would like to have more control over the URL. Therefore, I'm proposing to add an optional field to the remote object called filePath. If used, this field will control both the generated folder structure and the URL used to fetch the asset. It can be used as follows:

const filePath = getFilePath(...);
const fileHash = generateHashForFile(...);
remote: {
  enabled: isRemoteAsset,
  filePath: isRemoteAsset ? path.join(filePath, fileHash) : undefined,
  publicPath: getRemoteAssetsPublicPath(platform),
},

For example, assuming filePath is my/path/to/file and fileHash is abc123, the generated URL should be ${publicPath}/my/path/to/file/abc123/image.jpg, and this will also be the folder structure in the output folder.

Motivation

The goal is to address my use case where I need to inject a hash into the asset filename or place the asset in a folder named with the hash. I tried using the Output plugin to rename selected assets, but it did not affect the URL injected into the component pointing to the asset. In theory, I could just use publicPath for this and generate a different public path for every asset, but this would create a bit of a mess on the CDN. I created a small proof of concept to test if this is the right direction, although it is not ready to be merged. You can find more details here: https://github.com/adammruk/repack/pull/1.

Feel free to make any additional adjustments or let me know if I can solve my issue in a different way!

adammruk avatar May 22 '24 12:05 adammruk

Hi @adammruk,

I love the proposal, this is definitely a functionality that should be there! I'm not sure about the final API, hear me out:

Remote assets functionality is something that very much resembles asset/resource from Webpack, so I was thinking of aligning the API to utilise that. Take a look here: https://webpack.js.org/guides/asset-modules/#custom-output-filename and also here: https://webpack.js.org/configuration/module/#rulegeneratorfilename since these options are related. This also allows use to re-use the templating functionality from Webpack.

WDYT? Since this is a breaking change, it would require a new major, we could also implement an experimental API using something similar (or exactly like that) to what you've provided.

jbroma avatar May 23 '24 11:05 jbroma

Hi @jbroma, nice to talk with you again :D

Ok that looks super promising, I love the idea of reusing webpack templating! It crossed my mind while working on that if it would be possible. I'm not insisting on the API, the example was more about explaining the problem.

So, if I understand it correctly, we could have something like this:

remote: {
  enabled: isRemoteAsset,
  assetModuleFilename: "[path]/[name]-[hash].[ext]" 
  publicPath: getRemoteAssetsPublicPath(platform),
},

So the "assetModuleFilename" field will be applied only to remote assets. Why do you think this is a breaking change, since it will be a new field? It should be backwards compatibile, cause not specifying this field will just work the same as now. Od did I just misunderstood something πŸ˜…

adammruk avatar May 24 '24 05:05 adammruk

I was thinking even more than that: we could reuse the existing fields in webpack config without duplicating them - they only get applied to asset modules of type asset or asset/resource anyways, and these are not used atm on platforms like android and ios with Re.Pack.

We could go even a step further and override the asset/resource for android/ios platforms through createGenerator hook from Webpack (docs), so essentially running the that hook in RepackTargetPlugin.

jbroma avatar May 24 '24 10:05 jbroma

Sorry for such late response, I have been off for the whole week. I'm not super familiar with all of those stuff πŸ˜… But generally speaking this approach should solve our issue. Can you point me please some similar places in the code so I can take a look how this should be done?

adammruk avatar Jun 03 '24 12:06 adammruk

Hi @jbroma!

I have been playing around with it a bit, but I think I’m stuck. I tried to utilize assetModuleFilename in the webpack config, within the output field, but I think I’m missing a piece here. I think the main issue is that I don’t understand when and where the template passed to the assetModuleFilename field is actually transformed into the final path.

I feel like the logic should be inside Repack’s AssetsLoader because we are calling convertToRemoteAssets from there, so at that point, the final path should be available. Could you give me a few more hints on how to continue with that?

adammruk avatar Jun 12 '24 07:06 adammruk

Hi @adammruk, sorry for the the delayed response on my part this time!

there is no way to utilize assetModuleFilename right now, the assetLoader would need to become a replacement for asset generator (specified using type: asset/resource as an example, instead of use: '@callstack/repack/assetLoader'). Perhaps I went a little bit too far ahead in the concepts here πŸ˜…

Lets go back to the beginning, I think we should proceed with your original idea and just mark the API as unstable to leave some room for improvements and denote that explicitly. Perhaps a callback with some asset info like assetPath or assetData could prove useful for generating the desired filePath?

jbroma avatar Jun 12 '24 12:06 jbroma

That's fine:D Ok, so it is indeed not that easy πŸ˜… But anyway sounds like a nice final solution. So if it's fine for you, I can move forward with the initial idea. I have been experimenting with the callback function, but the problem is that we have a validation here: https://github.com/callstack/repack/blob/main/packages/repack/src/webpack/loaders/assetsLoader/options.ts and I cannot pass the function as a type for a field. I can't remember the original error but it was complaining that function type is not compatible with JSON Schema.

I'm thinking about the possibility to use webpack templating here, so we could pass something like this:

assetModuleFilename: "[path]/[name]-[hash].[ext]" 

to have at least a little bit of aligned API with webpack standards, but I'm not sure how to transform this template in the Repack's AssetsLoader to the "real" filename. WDYT?

adammruk avatar Jun 13 '24 08:06 adammruk

Ok I was able to find a solution, still testing it but initial checks looks promising, I updated my PR: https://github.com/adammruk/repack/pull/1/files What are your thoughts about it?

adammruk avatar Jun 17 '24 08:06 adammruk

Hi @adammruk

For the function type, I think we can use instanceOf: 'Function' instead of type like in packages/repack/src/webpack/plugins/CodeSigningPlugin/config.ts (there it's used for RegExp).

I'm not sure about using interpolateName from loader-utils since we've recently removed loader-utils as a dependency of Re.Pack in #628 - loader-utils might not be compatible with rspack in V5. I took a quick look and it should be fine but that might change in the future.

The rest of that looks good to me πŸš€, I guess we should open a PR a discuss implementation details there πŸ‘

jbroma avatar Jun 17 '24 12:06 jbroma

Ok great, thanks for the hints! πŸš€ I'm sitting on 3.4.0 in my project that's why I was tinkering with that version, was a bit easier to test for me. I'll switch now to some example project:)

{ instanceOf: 'Function'} is not throwing the error, so I'll try go with function approach. PR soon :D

adammruk avatar Jun 19 '24 05:06 adammruk

Hi @jbroma ! PR here: https://github.com/callstack/repack/pull/651 - let me know what do you think and what can be improved :)

adammruk avatar Jun 20 '24 06:06 adammruk

Hey! I was tinkering with hashes for remote assets as well and come up with the following hack πŸ˜… It for sure can be improved further. Maybe it's not worth calculating the content hash for each asset scale (if any) or even the hash can be calculated for the file modification date instead.

@callstack+repack+3.7.0.patch

lstoyanoff avatar Jun 28 '24 12:06 lstoyanoff

Hi @lstoyanoff ! I'm happy that I'm not the only one having this case πŸ˜… I agree, there are different usecases, technically different scale outputs a different content hash, but the general issue is to identify each asses, not necessary each scale. That's why I'm opting for more flexible approach, where you can decide it for yourself.

@jbroma any updates about that change?

adammruk avatar Jul 01 '24 06:07 adammruk

Hi @lstoyanoff ! I'm happy that I'm not the only one having this case πŸ˜… I agree, there are different usecases, technically different scale outputs a different content hash, but the general issue is to identify each asses, not necessary each scale. That's why I'm opting for more flexible approach, where you can decide it for yourself.

@jbroma any updates about that change?

feels like we should go with a flexible approach for now, we can always address things later if needed. @adammruk I've reviewed the PR πŸ‘

jbroma avatar Jul 02 '24 11:07 jbroma