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

fix: lazily resolve translate service in translate compiler #978

Merged
merged 2 commits into from
Jan 31, 2022

Conversation

dhhyi
Copy link
Collaborator

@dhhyi dhhyi commented Jan 12, 2022

PR Type

[x] Bugfix

What Is the Current Behavior?

translate functionality of PWATranslateCompiler breaks on SSR after the first SSR prerender.

Steps to reproduce:

  • add a translation key using the translate functionality:
    "test.translate.key": "Welcome to {{ translate, seo.defaults.description }}",
    
  • use this key on some page:
    {{ 'test.translate.key' | translate }}
  • start SSR: npm run start
  • trigger an SSR render for that page multiple times

-> it will work properly the first time, but for every subsequent prerender the following errors are displayed in the console:

ERROR Error Injector has already been destroyed. at R3Injector.assertNotDestroyed (/home/dh/workspace/intershop-pwa/dist/b2b/server/main.js:1:1357692)

What Is the New Behavior?

  • translate service is lazily fetched from the injector once and reused every time.

Does this PR Introduce a Breaking Change?

[ ] Yes
[x] No

Other Information

AB#72606

@dhhyi dhhyi marked this pull request as ready for review January 12, 2022 11:16
@dhhyi dhhyi added bug Something isn't working community Community contributions labels Jan 12, 2022
@dhhyi
Copy link
Collaborator Author

dhhyi commented Jan 12, 2022

not solved completely yet 😢
Going to a different page first leaves the issue happening.

@dhhyi dhhyi marked this pull request as draft January 12, 2022 15:14
@dhhyi dhhyi self-assigned this Jan 12, 2022
@dhhyi dhhyi force-pushed the fix/translate-key-translate-operation-on-ssr branch from 01d8680 to 35ee368 Compare January 12, 2022 15:32
@dhhyi dhhyi marked this pull request as ready for review January 12, 2022 15:32
@dhhyi
Copy link
Collaborator Author

dhhyi commented Jan 12, 2022

@MaxKless NOW it is ready 😄

@dhhyi dhhyi removed their assignment Jan 12, 2022
@MaxKless
Copy link
Collaborator

Sorry it took so long for me to review this. It looks good, but is there any more to the setup?
I can't seem to reproduce the original error on our current develop branch. Can you confirm the problem still exists @dhhyi ?

@dhhyi
Copy link
Collaborator Author

dhhyi commented Jan 26, 2022

@MaxKless Still happens for me. Try the following:

add new key to src/assets/i18n/en_US.json
echo "{{ 'test.translate.key' | translate }}" > src/app/pages/home/home-page.component.all.html
npm run start

other shell:
watch curl http://localhost:4200/home

Copy link
Collaborator

@MaxKless MaxKless left a comment

Choose a reason for hiding this comment

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

this.translate().instant() might look slightly weird, but imo it's a good, minimally invasive and unobtrusive change. Just a single comment.

src/app/core/utils/translate/pwa-translate-compiler.ts Outdated Show resolved Hide resolved
@MaxKless MaxKless self-requested a review January 31, 2022 17:43
@MaxKless MaxKless merged commit 3838171 into develop Jan 31, 2022
@MaxKless MaxKless deleted the fix/translate-key-translate-operation-on-ssr branch January 31, 2022 18:40
@shauke shauke added this to the 2.0 milestone Feb 3, 2022
SGrueber pushed a commit that referenced this pull request May 19, 2022
Co-authored-by: max.kless@googlemail.com <max.kless@googlemail.com>
SGrueber pushed a commit that referenced this pull request May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working community Community contributions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants