fix: message loading by fixing dependencies

[maribethb]

This is a patch release of #6219 into master.

#6210 was already merged into master, containing part one of the fix. We previously released a beta version of blockly 8.0.4-beta.1 that included both the patch in this PR and the previous patch, but we haven't yet released them as a minor release to widely fix the problems with message loading in #6123 and some of the problems reported in #4369 (though may not fix the React code splitting / lazy loading issues as that may be separate)

This does not update package.json because in master package.json already is set to version 8.0.4 and a previous PR included the rebuilt compressed files containing that version change.

[BeksOmega]

I think this branch picked up some erroneous sourcemap files that need to be removed.

Also, since this is being merged into master, I think the message files need to be rebuilt and added to the msg/ directory. But I'm not 100% sure of our policy on that.

[maribethb]

I ran

npm run clean
npm run build
npm run checkin:built

and these are the files that were changed. So I'm assuming the map files are legitimate changes that just weren't checked in during the last rebuild when #6210 (or another PR) was merged... Otherwise how would I know which files to merge and which not? If the map files are accurate to the current build why wouldn't I check them in (I'm not being snarky, I'm genuinely confused about how we decide to check in which files)

Similarly the message files being rebuilt. npm run build says it built the language files. checkin:built didn't update any files so I assumed they hadn't changed since #6210 . That seems accurate to me since the change in this PR only updates the files after they are wrapped in the umd wrapper which isn't checked in. Is there another command I should be running?

[BeksOmega]

I think your explanation of the map files makes sense. It's just weird to me that they weren't checked in last time (I would also expect them to be checked in at the same time as the compressed files). I was assuming that the ones in this PR were erroneously generated. But if they work then go for it!

Your explanation of message files also makes sense. I thought that we did check in the uml-wrapped versions, but I guess not.

But just to be clear, when they are changed, you need to manually copy them over :/ Afaik checkin:built does not handle the message files.

[maribethb]

I don't really have a good way of knowing if the map files work or not, other than trusting that our build scripts generate useful output. :/ sourcemaps still load though.

for messages, I don't think we have previously committed the wrapped versions, e.g. https://github.com/google/blockly/blob/c9329ea8eb39fa9b774ddc915b2357ec5137c6d1/msg/js/ab.js

Another difference is that in the msg directory the messages are split into js/ and json/ directories, but in dist/ the message files are just at the top level and with .js files only. So this matches that as well.

It's maybe kind of weird that we don't check in the packaged version, since we do with blockly_compressed et al. But this matches the historic behavior so I would rather keep that for now and consider changing it with a future mainline release.

And then final note is there is no diff between msg and build/msg so we are all good there (all the changes were checked in already with the previous PR)

So I think this is good to go but since I didn't verify this all until late this afternoon and I don't want to push on Friday we should do this on Monday morning.

[cpcallen]

So I'm assuming the map files are legitimate changes that just weren't checked in during the last rebuild when #6210 (or another PR) was merged... Otherwise how would I know which files to merge and which not? If the map files are accurate to the current build why wouldn't I check them in?

I concur that the map files should be checked in, as they have for other releases since they were first added in May 2020.

Similarly the message files being rebuilt. npm run build says it built the language files. checkin:built didn't update any files so I assumed they hadn't changed since #6210 . That seems accurate to me since the change in this PR only updates the files after they are wrapped in the umd wrapper which isn't checked in.

I concur.

Is there another command I should be running?

The "Day of Release" docs say to run npm run recompile and npm run release; I believe that is still correct. (It should not be necessary to run clean, build, or checkin:built manually.)

I would only add that it would be wise to run npm ci first, to make sure dependencies (including e.g. Closure Compiler) are at their specified versions. I've sent @maribethb a CL to modify the documentation accordingly.

But just to be clear, when they are changed, you need to manually copy them over :/ Afaik checkin:built does not handle the message files.

@BeksOmega: can you clarify what you mean here? It appears to me that ${BUILD_DIR}/msg/js/*.js is one of the things copied by checkinBuilt. Is that not working for some reason?

for messages, I don't think we have previously committed the wrapped versions

We have not.

It's maybe kind of weird that we don't check in the packaged version, since we do with blockly_compressed et al. But this matches the historic behavior so I would rather keep that for now and consider changing it with a future mainline release.

I think the preferable course of action for the future is to remove build output from the repository entirely. At the moment in the develop branch, npm run prepare (a side effect of npm install) is configured to do enough (e.g. run tsc and closure-make-deps) that uncompiled mode can be run without additional steps. It would be easy to configure it to additionally build the langfiles or even do a full Closure Compile if desired.

So I think this is good to go but since I didn't verify this all until late this afternoon and I don't want to push on Friday we should do this on Monday morning.

I concur, bar one very small nit: shouldn't the title of this PR (and the resulting commit) be "release: …", not "fix: …"?

[BeksOmega]

@BeksOmega: can you clarify what you mean here? It appears to me that ${BUILD_DIR}/msg/js/*.js is one of the things copied by checkinBuilt. Is that not working for some reason?

Our internal docs say they need to be copied over manually, so I was just assuming that message files were not handled. If that is incorrect could you update the docs?

[maribethb]

I'm creating a patch release, so I can't run recompile because I don't want to sync with develop upstream. We might have been able to use release still (I'm not sure tbh) but since we already updated the version in package.json previously to the one we want it to be, I won't.

And this isn't really a release type PR because, to me, it fixes a bug rather than does some release-specific chore like the "merge all of develop into master" PRs do. It could be argued either way but since this fixes a bug I'm just gonna leave it as fix, which I think would make the most sense from a release notes perspective.

[cpcallen]

Our internal docs say they need to be copied over manually, so I was just assuming that message files were not handled. If that is incorrect could you update the docs?

I think the language recompilation procedure requires a rethink, because the translatewiki branch diverged from develop exactly a year ago today and has received no updates (except from TranslateWiki) since then. I have filed issue #6312 to discuss how to resolve this (now and going forward).

I'm creating a patch release, so I can't run recompile because I don't want to sync with develop upstream.

Oh, good point.

We might have been able to use release still (I'm not sure tbh) but since we already updated the version in package.json previously to the one we want it to be, I won't.

Yeah, that would have been preferable: checking package_tasks.js, I see that npm run publish also runs the rebuildAll function, which is the main thing that I wanted to ensure had happened.