fix(rosetta): infuse incorrectly handles compressed assemblies #3669
Conversation
| { compress = false }: { compress?: boolean } = {}, | ||
| ) { | ||
| writeAssembly(directory, _fingerprint(assembly), { compress }); | ||
| export function replaceAssembly(assembly: spec.Assembly, directory: string) { |
There was a problem hiding this comment.
This is a breaking change :(. If we must add the { compress = false }: { compress?: boolean } = {} option in again, then fine. I really don't want to though, because in my mind, this was a bit of a stealth mode release and its unlikely that this revert actually breaks any users. Especially because the prior behavior was incorrect, and I would simply ignore the property if we have to keep it in...
There was a problem hiding this comment.
You could also just change the default behavior for compress -- JavaScript does not require default values are constants/literals, it can be a function call result...
|
Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it! |
|
Merging (with squash)... |
The behavior of
rosetta infusewas incorrectly handled before.infusewould always overwrite the.jsiifile with the uncompressed assembly. This PR fixes that behavior by detecting whether or not there is a compressed file in the directory, and compressing if that is the case.There are two alternative solutions I considered, primarily because I'm concerned that looking up the location of the compressed assembly can be considered a leaky abstraction:
.jsiiagain to determine whether or not it is a file redirect. I did not choose this option as it involves additional loading, which will slow things down.LoadedAssemblytype to include information on whether or not the assembly was originally compressed. Then pass that into thereplaceAssembly()function. I ultimately decided against this because it would involve changing the function signature of allloadAssemblyFromXxxfunctions. It's both a breaking change, and unnecessary clutter for a single use case.Based on the reasoning above, I think what is included in this PR makes the most sense: expose an independent function,
compressedAssemblyExists, that returns whether or not there is a file located atSPEC_FILE_NAME_COMPRESSED.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.