-
Notifications
You must be signed in to change notification settings - Fork 482
feat(universal): Allow for configuring if preboot complete() is called automatically (immediatelly) or manually (asynchronously) #552
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
1 similar comment
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
CLA signed 👍 |
CLAs look good, thanks! |
1 similar comment
CLAs look good, thanks! |
…d automatically (immediatelly) or manually (asynchronously) This is useful if you need to do anything asynchronous (e.g. fetch data) before preboot complete() is called.
6e44f87
to
e10b5ac
Compare
@steve8708 sorry, can you change this so that it auto completes by default? Right now if you don't set autoPreboot then it won't complete. This will break everyone. |
Also erroring here in Travis
|
@@ -77,15 +78,22 @@ export const UNIVERSAL_CACHE = new OpaqueToken('UNIVERSAL_CACHE'); | |||
deps: [] | |||
}, | |||
{ | |||
provide: AUTO_PREBOOT, | |||
useValue: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeffwhelpley shouldn't this default the autoPreboot to true
below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, including the token in the default providers array here allows the default behavior to remain unchanged.
anyone who doesn't provide a value for this token will get the default value true
which will keep the current behavior the same for current universal users
only if someone explicitly provides a value for this token or provides a value for autoPreboot in the config object of UniversalModule.withConfig does this get overridden
@ca136 not if you don't set the AUTO_PREBOOT token. You are introducing that with this change, so when someone else upgrades, they won't have that set and will thus not have autoPreboot set even though they technically did have it before. Typically we want to try and ensure that when changes are introduced someone can upgrade without adversely affecting behavior. If you upgraded to this but didn't change anything else, preboot would go from automatically completing to not automatically completing. |
LGTM |
thanks @gdi2290 👍 @MarkPieszak looks like that tsc error was already on development but added another commit to this PR that resolves the error 👌 |
@steve8708 with this PR I disabled the preboot.complete, but how it can be called now manually? The import of prebootClient from the old versions dont work anymore. |
@andresantos123 come to think of it it may be nice to optionally export prebootComplete again like we did before In the meantime you can see how we get the preboot client in universal here (A little ugly unfortunately, I believe it is this way to support a breaking API change among different versions of the preboot library) and we call it here; So you should be able to do similar var prebootClient;
try {
prebootClient = require('preboot/__build/src/browser/preboot_browser');
prebootClient = (prebootClient && prebootClient.prebootClient) || prebootClient;
} catch (e) {}
// .. and when you are ready
prebootClient().complete() |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Feature
preboot complete() is called immediately upon client bootstrap, wether you want it to be or not
Allows a configuration to override the automatic prebooting. This is especially useful is you have to do anything asynchronous before preboot complete runs, such as fetching data.
Usage is
No
Open to any feedback on other ways to implement this feature and if good where to add tests and docs. Thanks!
…