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: add support for clientData #1286

Closed
wants to merge 4 commits into from
Closed

feat: add support for clientData #1286

wants to merge 4 commits into from

Conversation

Mister-Hope
Copy link
Member

No description provided.

@Mister-Hope

This comment was marked as outdated.

@Mister-Hope Mister-Hope marked this pull request as ready for review April 21, 2023 06:08
@Mister-Hope
Copy link
Member Author

Mister-Hope commented Apr 21, 2023

@meteorlxy This pr just include clientData api itself now, if you think that the api is ready, we can release this in next version so that I can start to use it and collect some feedbacks from my users.

We can just leave it undocument, and if you want, I can add some docs and mark them as W.I.P.

@meteorlxy
Copy link
Member

@Mister-Hope In fact, I don't see too many differences from provide/inject:

export default defineClientConfig({
  enhance({ app }) {
    app.provide(foo, bar)
  },
})

Any further ideas?

@Mister-Hope
Copy link
Member Author

Mister-Hope commented Apr 24, 2023

@Mister-Hope In fact, I don't see too many differences from provide/inject:

export default defineClientConfig({
  enhance({ app }) {
    app.provide(foo, bar)
  },
})

Any further ideas?

This ensures that the clientData can be visited in any enhance function.

There will be cases where a plugin is providing some exports which requires client data.

I make a draft with the code you wrote in my theme, and I met similar problems. The theme clientConfig seems not be the first one, so I cannot access the data in enhance cycle in some cases.

Moving it to core insure every data is injected before any client hook.

@Mister-Hope
Copy link
Member Author

Any update on this?

This ensures that the clientData can be visited in any enhance function.

There will be cases where a plugin is providing some exports which requires client data.

I make a draft with the code you wrote in my theme, and I met similar problems. The theme clientConfig seems not be the first one, so I cannot access the data in enhance cycle in some cases.

Moving it to core insure every data is injected before any client hook.

I think this reason is enough to add this feature in core.

@Mister-Hope
Copy link
Member Author

cc again @meteorlxy

@meteorlxy
Copy link
Member

A beforeEnhance option might be more general?

@Mister-Hope
Copy link
Member Author

Mister-Hope commented Feb 22, 2024

I think a beforeEnhance option is pretty weird, as I am not thinking out any other things this option can solve.

Let's focus on the problem of this pr:

  • User should be allowed to set options in client side
  • All plugins should be able to read these client configs, so that users do not need to set repeatly with these configs
  • The currently implementation is that a plugin manually provide its own config within enhance lifecycle, so that the plugin sequence decides the client config scope where a plugin or theme could access

Adding beforeEnhance could be a solution, but if there is no other use for it, a normal developer or users which does not need a client config could be confused about the hook.

Adding it to client as a new concept seems more natural to me, as the client is handling the "client config" before any existing hooks to let it be accessable.

Another improvement is that we now requires users to add a export default {}.

We could also allow the user client config to do not have any default export, i.e.:

// .vuepress/client.ts
// imports

definePluginAConfig({ xxx })

@Mister-Hope
Copy link
Member Author

Another improvement is that we now requires users to add a export default {}.

We could also allow the user client config to do not have any default export, i.e.:

// .vuepress/client.ts
// imports

definePluginAConfig({ xxx })

I would like to open a pr for this, this is useful for users that only want to add this file for plugin customization purpose, it will be a bit strange for them to understand that without a default export we are throwing an error.

Also, A lot of plugins are not expecting usage with the export object, maybe they just want to add some styles or library imports on the client side.

@meteorlxy meteorlxy deleted the clientData branch July 15, 2024 08:46
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