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

Make PrettyTime objects @Dependent #6

Closed
gastaldi opened this issue Mar 11, 2021 · 11 comments
Closed

Make PrettyTime objects @Dependent #6

gastaldi opened this issue Mar 11, 2021 · 11 comments

Comments

@gastaldi
Copy link
Member

Because PrettyTime objects are mutable, it doesn't make sense to have it as a Singleton, since it may bring concurrency issues when the object state is changed in separate threads

@mkouba
Copy link
Contributor

mkouba commented Mar 11, 2021

I'm not quite sure that @Dependent solves the problem.

  1. I don't see a point of doing @Inject PrettyTime vs new PrettyTime()
  2. You can still inject a PrettyTime instance in a @Singleton or @ApplicationScoped bean and the problem may appear again...
  3. The combination of @Named and @Dependent is inefficient (e.g. every expression receives a new dependent object)

@gastaldi
Copy link
Member Author

I can understand that keeping it as a Singleton and leave it up to the user to decide if its state works for them or not is okay, but how do we go with different locales x Named in a Qute template?

@mkouba
Copy link
Contributor

mkouba commented Mar 11, 2021

but how do we go with different locales x Named in a Qute template?

That's a good point. We don't ;-). We would need a proper Qute integration, e.g. a namespace resolver that can access the locale attribute of a TemplateInstance... {prettyTime:format(time)}.

@gastaldi
Copy link
Member Author

One workaround is having the application produce their own named prettyTime objects, since the existing producer is a @DefaultBean.

How do you suggest we go from here?

@gastaldi
Copy link
Member Author

At the moment I am leaning towards leaving the injected PrettyTime as a singleton and let the user aware that changing its state will affect all uses of that object.

@gsmet any thoughts?

@gastaldi
Copy link
Member Author

Added note to README

@gastaldi
Copy link
Member Author

Also created ocpsoft/prettytime#211 asking PrettyTime to be immutable

@gsmet
Copy link
Member

gsmet commented Mar 11, 2021

I don't know. I hate to say this but I don't really see the point of this extension given this thing is not immutable and apparently it doesn't need anything to work in native mode.

@gastaldi
Copy link
Member Author

gastaldi commented Mar 11, 2021

it doesn't need anything to work in native mode

@gsmet this is absolutely not true, we need this snippet for the extension to work in native mode:

https://github.com/quarkiverse/quarkus-prettytime/blob/main/deployment/src/main/java/io/quarkiverse/prettytime/deployment/PrettyTimeProcessor.java#L18-L21

See the changes in quarkusio/status.quarkus.io#31

@gastaldi
Copy link
Member Author

Also as I explained in the README, it's fine to not be immutable if the use case is to have a PrettyTime object that will be reused among the entire application (as in the tests - with a fixed locale)

@mkouba
Copy link
Contributor

mkouba commented Mar 12, 2021

I think that it's OK if we document that the shared PrettyTime instance is initialized with the default locale and the System#currentTimeMillis() is used as the reference point.

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

No branches or pull requests

3 participants