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

string plugin getWithCache doesn't really cache that much #797

Closed
zepumph opened this issue Sep 23, 2019 · 4 comments
Closed

string plugin getWithCache doesn't really cache that much #797

zepumph opened this issue Sep 23, 2019 · 4 comments

Comments

@zepumph
Copy link
Member

zepumph commented Sep 23, 2019

While working on phetsims/rosetta#193, I found that almost every string being required from the sim was triggering a loading of the string file. I think this is because all the require loads occur before the text plugin has async come back with the JSON.

A good way to test this problem is to put console.log('url') in the text.get() callback.

@zepumph
Copy link
Member Author

zepumph commented Sep 23, 2019

Above I added a way to cache the intermediary state before the first loading of a string file. I'm pretty happy with this change. It is simple, and is a definite improvement in my book. @samreid can you please review.

@samreid
Copy link
Member

samreid commented Sep 24, 2019

This looks great and looks like it cures the symptoms described in #730.

Some questions and comments:

  • {Object.<url:string, Array.<function>} is missing a >
  • Why does action => action && action() check action &&?
  • In action => action && action() it seems better to pass through parsed directly like action => action && action(parsed), instead of relying on the closure variable to be correct in the arrow function. Would that work?

@zepumph
Copy link
Member Author

zepumph commented Sep 24, 2019

Thanks! I really like the suggestions. How's that?

@zepumph zepumph assigned samreid and unassigned zepumph Sep 24, 2019
@samreid
Copy link
Member

samreid commented Sep 24, 2019

Looks great, thanks! I'm ready to close if you are.

@zepumph zepumph closed this as completed Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants