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

Force check i18n cache when cleaning project #4474

Merged
merged 1 commit into from
Jan 14, 2022

Conversation

fluiddot
Copy link
Contributor

@fluiddot fluiddot commented Jan 14, 2022

Following up #4469, this PR includes the command npm run i18n:check-cache in i18n cache cleaning command (npm run clean:i18n). This way we assure that the i18n cache is always present, which is required for generating the JS bundle.

Thanks @guarani for spotting this and suggestion the change in #4469 (comment) 🙇 .

To test

Clean command recreates i18n cache

  1. Run command npm run clean.
  2. Observe that the folder src/i18n-cache is removed when the command is being executed.
  3. Observe that the folder src/i18n-cache is created again.

JS bundle is generated properly

  1. Run command npm run bundle.
  2. Observe that the command is executed properly without errors.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes more info and have added them to RELEASE-NOTES.txt if necessary.

@fluiddot fluiddot added the [Type] Bug Something isn't working label Jan 14, 2022
@fluiddot fluiddot requested a review from guarani January 14, 2022 20:30
@fluiddot fluiddot self-assigned this Jan 14, 2022
Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix here, @fluiddot!

The test cases worked properly on my machine; however, the npm run bundle command fails when the bundle is not present. For example:

rm -rf bundle
npm run bundle

fails (I unfortunately didn't capture the error, but will add it here if I get the chance to re-run today).

Since this fixes the most common case where a bundle exists and it is re-generated, I think it's good to approve and merge, which I'll do now.

@guarani guarani merged commit c02c750 into develop Jan 14, 2022
@guarani guarani deleted the fix/clean-i18n-cache-command branch January 14, 2022 21:14
@fluiddot
Copy link
Contributor Author

The test cases worked properly on my machine; however, the npm run bundle command fails when the bundle is not present. For example:

rm -rf bundle
npm run bundle

fails (I unfortunately didn't capture the error, but will add it here if I get the chance to re-run today).

@guarani I think removing the bundle folder is not expected nor required when generating a bundle, as that folder is currently versioned. In what case do you think we would need to remove it?

@fluiddot fluiddot mentioned this pull request Jan 17, 2022
2 tasks
@guarani
Copy link
Contributor

guarani commented Jan 17, 2022

@guarani I think removing the bundle folder is not expected nor required when generating a bundle, as that folder is currently versioned. In what case do you think we would need to remove it?

It's my understanding that npm run bundle – which updates (or creates) the bundle - used to handled the scenario where the bundle is not present (please correct me if I'm wrong here) and this changed recently.

It's often the case when debugging an issue that we delete build artifacts, derived data, etc and I think that includes the bundle. (The only reason I'm aware of that we check-in the bundle is because we don't want the main apps to have the tooling needed to build it.)

If for example, I had an issue where the main apps didn't seem to pick up an updated bundle, personally I probably would delete the bundle and generate it again to make sure nothing was being cached. If I was unaware of how npm run bundle generates an error when a bundle isn't present, I might be slowed down looking into the error.

@guarani
Copy link
Contributor

guarani commented Jan 17, 2022

Do you happen to know why the bundle command would require an existing bundle?

@fluiddot
Copy link
Contributor Author

Ok, I see your point and totally agree with the fact that deleting the bundle shouldn't produce issues on the generation command, as it's an artifact only required by the integration with the main apps 👍 . In fact, I don't think that the bundle or any asset file contained within that folder (i.e. bundle) is required by any command 🤔 , I'll do a quick check to identify the script that is failing.

@fluiddot
Copy link
Contributor Author

Do you happen to know why the bundle command would require an existing bundle?

Looks like it's failing in the scripts that generate the localization strings files (reference). However, the cause is just the fact they expect the bundle folder to present, so this could be easily addressed by assuring that the folder is created in case it doesn't exist, I'll prepare a PR to correct this 🔧 .

@fluiddot
Copy link
Contributor Author

@guarani I've just created this PR, in order to address this issue, and assigned you as a reviewer. Let me know if you could take a look, thanks 🙇 !

@guarani
Copy link
Contributor

guarani commented Jan 17, 2022

Looks like it's failing in the scripts that generate the localization strings files (reference). However, the cause is just the fact they expect the bundle folder to present, so this could be easily addressed by assuring that the folder is created in case it doesn't exist, I'll prepare a PR to correct this 🔧 .

Thanks for pinpointing the cause there 🙇

@guarani I've just created this PR, in order to address this issue, and assigned you as a reviewer. Let me know if you could take a look, thanks 🙇 !

Sure, will take a look, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants