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

feat: 🎸 message-format cache compiled messages #360

Closed
wants to merge 1 commit into from
Closed

feat: 🎸 message-format cache compiled messages #360

wants to merge 1 commit into from

Conversation

k3nsei
Copy link

@k3nsei k3nsei commented Oct 9, 2020

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[x] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Message format compile is always used.

Issue Number: #358

What is the new behavior?

Message format use compile only once per message.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

none

@k3nsei k3nsei changed the title feat: 🎸 Cache compiled messages feat: 🎸 message-format cache compiled messages Oct 9, 2020
@shaharkazaz shaharkazaz requested a review from itayod October 24, 2020 11:03
@itayod
Copy link
Contributor

itayod commented Oct 27, 2020

Hi @k3nsei LGTM, but I don't think we want that as default behavior, so let's add it as an option in the plugin's configuration and make it "false" by default.

@shaharkazaz
Copy link
Collaborator

shaharkazaz commented Nov 28, 2020

@k3nsei Hey! will you have the time to make the change requested by @itayod so we continue with this PR? 🙂

@k3nsei
Copy link
Author

k3nsei commented Nov 28, 2020

@shaharkazaz yes I will do that.

MessageFormatTranspiler transpile method compile messages to fn everytime it has ben called. Those generated fns can be cached and reused. Because result is always the same. Only parameters which those fns take can vary.

Signed-off-by: Piotr Stępniewski <[email protected]>
@itayod
Copy link
Contributor

itayod commented Nov 30, 2020

Hi @k3nsei, thanks for the update.

  • can you add this optimization to the plugin's config and make it false by default (as I wrote above).
  • if you may, you can add it to the documentation.
  • please don't force push it makes it hard to track the changes :)

Thanks!

@itayod
Copy link
Contributor

itayod commented Dec 23, 2020

@k3nsei Hey!
I really want to push this forward 🙏 (and add you as a contributor! 🥳 )

These are the fixes needed:

  • Make this optimization configurable as part of the config passed to the module with the default of false( we will make it true by default in the next major version, but we don't want any side effects now).
  • Update the docs accordingly.
  • Add short tests making sure it's working as expected.

We're here if you need assistance. Let's merge this thing. 💪

@k3nsei
Copy link
Author

k3nsei commented Jan 22, 2021

@itayod You can pick it up. I can't find free time to move it forward.

@itayod
Copy link
Contributor

itayod commented Jan 26, 2021

@itayod You can pick it up. I can't find free time to move it forward.

No rush, feel free to continue work on this when you can.

@shaharkazaz
Copy link
Collaborator

Added in v3

@zhienbaevsa
Copy link

zhienbaevsa commented Jan 30, 2022

@shaharkazaz @k3nsei Hi! What is the fate of this feature request?
UPD: I found that you included this to 3 version in commit 773bc5c

@k3nsei k3nsei deleted the feature/message-format-cache-compiled-msgs branch January 31, 2022 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants