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

beforeCreateVueInstance can now return a promise #243

Closed
wants to merge 4 commits into from
Closed

beforeCreateVueInstance can now return a promise #243

wants to merge 4 commits into from

Conversation

rob-64
Copy link

@rob-64 rob-64 commented Jan 19, 2021

Allows developer to perform any async calls to init the root vue opts

@rob-64
Copy link
Author

rob-64 commented Jan 21, 2021

@karol-f any chance of reviewing this?

@karol-f karol-f self-requested a review January 21, 2021 20:31
@karol-f
Copy link
Owner

karol-f commented Jan 21, 2021

Can You please describe what You want to achieve? What problem does this solution solve?

@rob-64
Copy link
Author

rob-64 commented Jan 21, 2021

Can You please describe what You want to achieve? What problem does this solution solve?

I have a very unique use case where I need to fetch some instanced props and initial vuexState during Vue init... so by returning a promise, I can use an ajax call to get what I need from the server

@karol-f
Copy link
Owner

karol-f commented Jan 21, 2021

Ok, so why not making a beforeCreateVueInstance a Promise? Will it work? Seems a little bit cleaner.

@rob-64
Copy link
Author

rob-64 commented Jan 21, 2021

Ok, so why not making a beforeCreateVueInstance a Promise? Will it work?

Thats what I did... it can now return the Options or a Promise

@karol-f or maybe I misunderstand what you're saying

@karol-f
Copy link
Owner

karol-f commented Jan 22, 2021

Hi, checking Your PR, I can see that You are modifying createVueInstance and not beforeCreateVueInstance - that's why I'm asking.

I didn't analyse if solution You've introduced is better one. I'm still not sure if it's necessary. At least in that form.
screenshot_346
screenshot_347

@rob-64
Copy link
Author

rob-64 commented Jan 22, 2021

Hi, checking Your PR, I can see that You are modifying createVueInstance and not beforeCreateVueInstance - that's why I'm asking.

I didn't analyse if solution You've introduced is better one. I'm still not sure if it's necessary. At least in that form.
screenshot_346
screenshot_347

Gotcha, so I made it so that the function beforeCreateVueInstance can return either the options or a promise of the options.

So then inside the util function of createVueInstance I chain off of the promise and return true once the options are returned and then the vue instance is created. That way I only invoke the vueInstanceCreatedCallback once its truly done.

image

image

@rob-64
Copy link
Author

rob-64 commented Jan 25, 2021

@karol-f does that make sense what my plan was? I still don't understand what you are suggesting.

@karol-f
Copy link
Owner

karol-f commented Jan 25, 2021

Hi, I am currently a little busy, I will try to get back to it as soon as possible.

@rob-64
Copy link
Author

rob-64 commented Feb 8, 2021

Hi, I am currently a little busy, I will try to get back to it as soon as possible.

Hi @karol-f, any chance we could continue this PR?

@rob-64
Copy link
Author

rob-64 commented Feb 25, 2021

@karol-f Can we review this PR again?

I updated createVueInstance to return a Promise so I can chain it with the Promise now returned by beforeCreateVueInstance....

@karol-f karol-f self-assigned this Feb 25, 2021
@karol-f
Copy link
Owner

karol-f commented Feb 27, 2021

Hi, thank You for the PR. Sorry for the long wait. It looks good!

@karol-f karol-f closed this Feb 27, 2021
@karol-f
Copy link
Owner

karol-f commented Feb 27, 2021

I've copied Your changes (not merging the branch) as some changes (like dist or docs updating) I do during release. I hope it's ok with You.

@karol-f karol-f reopened this Feb 27, 2021
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.

2 participants