-
Notifications
You must be signed in to change notification settings - Fork 12
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: improvements #8
Conversation
|
||
export default (ctx, inject) => { | ||
inject('moment', moment) |
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.
This will introduce unnecessary extra load to each SSR request.
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.
So you'd vote to not inject
moment (but using Vue.prototype
instead)?
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.
Yes. I'm aware of the nuxt reload problem. Because of shared Vue instance, any config change for moment module is not being applied unless stop/start nuxt again. Anyway, I think it worth to fix the root problem instead of making production builds slower. We have 3 options:
- For now, use
inject
to prevent that problem and accept extra load until Nuxt.js fix - Use a conditional if for the template to use non-inject method for production only
- Just making users aware of such development inconsistency and keep it as is
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.
Because several issues stated that ppl want to use it with ctx
, so also on server side.
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.
@manniL Honestly this was not the purpose of this module and moment can be easily imported
whenever needed which is fully SSR safe too. BTW I agree with that. Let's keep using inject :)
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.
Other changes LGTM
@manniL I'll fix tests on master as they are failing there too |
How to use this module inside a store? |
Key features:
inject
defaultLocale
optionResolves #6
Resolves #5
Resolves #4
Resolves #3