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

fix: add functions to genRawValue #61

Closed
wants to merge 2 commits into from
Closed

fix: add functions to genRawValue #61

wants to merge 2 commits into from

Conversation

ineshbose
Copy link

@ineshbose ineshbose commented Feb 10, 2023

inspired from nuxt-modules/i18n#1617

This was the behaviour earlier:

console.log(genObjectFromRaw({ test: '() => import("pkg")', hello () { return "hi" } }))
// { test: () => import("pkg"), hello: hello () { return "hi" } } // SYNTAX ERROR

This is the behaviour after this PR:

console.log(genObjectFromRaw({ test: '() => import("pkg")', hello () { return "hi" } }))
// { test: () => import("pkg"), hello: function () { return "hi" } }

This is using regex, and I'm really happy to address cases I may have missed.

(PS. Could we consider also exporting genRawValue too please?)

@ineshbose
Copy link
Author

Hi, any reviews on this please? 😬

@pi0
Copy link
Member

pi0 commented Mar 17, 2023

Serializing a function in general is super tricky concept and i prefer not to support/adopt it here. Transforms (via runtime/buildtime) and global variable dependency are some examples of problems that can occur. Is nuxt/i18n supporting function serialization from nuxt.config to runtime?

@ineshbose
Copy link
Author

I'm in the process of investigating all function options in the vue-i18n ecosystem and helping out kazupon (massive respect to him) in the Nuxt adoption. Some have been moved to runtime hooks (nuxt-modules/i18n#1919), while I see that there's also a runtime plugin vueI18n also taking in functions from the config, so a template needs to be added.

I've discovered addPluginTemplate that takes options parameter, so I'm hoping that serializes the functions into the template - but I'd understand if that's not the case.

@ineshbose
Copy link
Author

For reference about serialization in the i18n module:

https://github.com/nuxt-modules/i18n/blob/8badc1a68057730dca83842771ef0bb6e9798952/src/gen.ts#L236-L238

@pi0
Copy link
Member

pi0 commented Mar 17, 2023

Thanks for sharing context. I wasn't aware. We probably have to also avoid this kind of serilization for i18n module too considering we are not doing this as best practice for Nuxt 3 itself. /cc @danielroe wdyt shall we support with warn or not?

@ineshbose
Copy link
Author

I've moved the root config functions to hook, but the vue-i18n plugin takes functions (read https://vue-i18n.intlify.dev/guide/essentials/syntax.html#custom-modifiers). Open to have a discussion about this over pm / discord. (and also involving kazupon if required)

@ineshbose
Copy link
Author

Closing this as I completely agree with the reasons of serialising functions to be extremely difficult so I don't want to encourage supporting it - I'm brainstorming & testing ideas to ensure there are no fragile behaviour for the module and so I'm feeling certain that I'll progress around it. 🙂

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