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

Add link to globalize-express to README.md #435

Closed
wants to merge 0 commits into
from

Conversation

Projects
None yet
4 participants
@devangnegandhi

I have written an ExpressJS middleware that makes it easy for developers of web applications to use Globalize within their express app. You can checkout the project here - https://github.com/devangnegandhi/globalize-express.

I think this is a very useful expressJS middleware that will benefit lot of developers that want to use Globalize in their web applications. The project is well tested and I feel adding a link to this on the jquery/Globalize github page would help people discover it.

@devangnegandhi

This comment has been minimized.

Show comment
Hide comment
@devangnegandhi

devangnegandhi Apr 23, 2015

I am not sure what component name should I give for the build to pass?

I am not sure what component name should I give for the build to pass?

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Apr 23, 2015

Member

You can use Docs:

Member

rxaviers commented Apr 23, 2015

You can use Docs:

@devangnegandhi

This comment has been minimized.

Show comment
Hide comment
@devangnegandhi

devangnegandhi Jul 17, 2015

Hey, has anyone from the globalize team got a chance to review this? Please feel free to let me know if there is any changes I should make. Thanks

Hey, has anyone from the globalize team got a chance to review this? Please feel free to let me know if there is any changes I should make. Thanks

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Jul 17, 2015

Member

@devangnegandhi sorry for the delay. Can you include in examples/ a full app example?

Member

rxaviers commented Jul 17, 2015

@devangnegandhi sorry for the delay. Can you include in examples/ a full app example?

@devangnegandhi

This comment has been minimized.

Show comment
Hide comment
@devangnegandhi

devangnegandhi Jul 18, 2015

@rxaviers Yes I can do that. I will add that and update the pull request.

@rxaviers Yes I can do that. I will add that and update the pull request.

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Aug 4, 2015

Member

This PR might fix #460.

Member

rxaviers commented Aug 4, 2015

This PR might fix #460.

@jquerybot jquerybot added CLA: Error and removed CLA: Valid labels Sep 18, 2015

@devangnegandhi

This comment has been minimized.

Show comment
Hide comment
@devangnegandhi

devangnegandhi Sep 28, 2015

@rxaviers A quick question. I have added a full app example in my repository (see https://github.com/devangnegandhi/globalize-express/tree/master/example). Do you want me to copy that same directory into this repo (which will cause me to maintain it at two places) or can I just link the example to my repo directly?

@rxaviers A quick question. I have added a full app example in my repository (see https://github.com/devangnegandhi/globalize-express/tree/master/example). Do you want me to copy that same directory into this repo (which will cause me to maintain it at two places) or can I just link the example to my repo directly?

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Oct 1, 2015

Member

@devangnegandhi linking is fine.

Member

rxaviers commented Oct 1, 2015

@devangnegandhi linking is fine.

@dpolivy

This comment has been minimized.

Show comment
Hide comment
@dpolivy

dpolivy Oct 13, 2015

@devangnegandhi nice to see someone else try to solve this problem, too! I also wrote my own express/globalize integration (https://github.com/dpolivy/express-globalize/); it seems we have some similarities and also some different approaches. I also wrote a handlebars module that plugs in using the same mechanism (https://github.com/dpolivy/handlebars-globalize).

@rxaviers I think it's great to add the related modules to the readme. I should probably create a PR to do that for my modules, and also finally add them to npm (I haven't been able to get back to this since I originally wrote them). The challenge is, these are similarly named, and solve similar problems. Is that a good thing or a bad thing for Globalize?

I haven't had a chance to do a thorough review of the code, though. At a high level, it looks fine. I haven't tried to run it. At a functional level, it is missing some features that I personally find important (ability to pick language based on server side user data, caching, etc), but that doesn't necessarily impact the utility for others.

dpolivy commented Oct 13, 2015

@devangnegandhi nice to see someone else try to solve this problem, too! I also wrote my own express/globalize integration (https://github.com/dpolivy/express-globalize/); it seems we have some similarities and also some different approaches. I also wrote a handlebars module that plugs in using the same mechanism (https://github.com/dpolivy/handlebars-globalize).

@rxaviers I think it's great to add the related modules to the readme. I should probably create a PR to do that for my modules, and also finally add them to npm (I haven't been able to get back to this since I originally wrote them). The challenge is, these are similarly named, and solve similar problems. Is that a good thing or a bad thing for Globalize?

I haven't had a chance to do a thorough review of the code, though. At a high level, it looks fine. I haven't tried to run it. At a functional level, it is missing some features that I personally find important (ability to pick language based on server side user data, caching, etc), but that doesn't necessarily impact the utility for others.

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Nov 4, 2015

Member

@devangnegandhi thanks for fixing the example. Could you rebase you PR please? Thanks

Member

rxaviers commented Nov 4, 2015

@devangnegandhi thanks for fixing the example. Could you rebase you PR please? Thanks

@devangnegandhi

This comment has been minimized.

Show comment
Hide comment
@devangnegandhi

devangnegandhi Nov 16, 2015

My commits in my fork got messed up for some reason. I will create a new PR. Sorry for this!!

@dpolivy Thanks for your feedback. Especially about picking languag based on server side user data. That is something I will think about incorporating this. Looking at the approach used in your library, it looks like you have wrapped all the methods in the globalize library. I am not sure how maintainable that is over time. I, on the other hand, have simply returned the globalize object for a specific locale. But like you said - that does not impact the utility for others :).

My commits in my fork got messed up for some reason. I will create a new PR. Sorry for this!!

@dpolivy Thanks for your feedback. Especially about picking languag based on server side user data. That is something I will think about incorporating this. Looking at the approach used in your library, it looks like you have wrapped all the methods in the globalize library. I am not sure how maintainable that is over time. I, on the other hand, have simply returned the globalize object for a specific locale. But like you said - that does not impact the utility for others :).

@dpolivy

This comment has been minimized.

Show comment
Hide comment
@dpolivy

dpolivy Nov 16, 2015

@devangnegandhi I chose to wrap the methods in order to implement caching of created formatters to improve performance. It's certainly not ideal as you note, but until that is baked into globalize itself, it's a necessary performance improvement for my scenarios, at least! Thanks for taking a look!

dpolivy commented Nov 16, 2015

@devangnegandhi I chose to wrap the methods in order to implement caching of created formatters to improve performance. It's certainly not ideal as you note, but until that is baked into globalize itself, it's a necessary performance improvement for my scenarios, at least! Thanks for taking a look!

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Nov 16, 2015

Member

@dpolivy is there something that prevents you from using @devangnegandhi's solution, or vice-versa @devangnegandhi? I encourage you both to join efforts since you're addressing the same problem.

Member

rxaviers commented Nov 16, 2015

@dpolivy is there something that prevents you from using @devangnegandhi's solution, or vice-versa @devangnegandhi? I encourage you both to join efforts since you're addressing the same problem.

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Nov 16, 2015

Member

I chose to wrap the methods in order to implement caching of created formatters to improve performance. It's certainly not ideal as you note, but until that is baked into globalize itself

Caching support is ready to land at #470. Basically, it just awaits for more eyeballs to review and make sure it addresses real cases like this one.

Member

rxaviers commented Nov 16, 2015

I chose to wrap the methods in order to implement caching of created formatters to improve performance. It's certainly not ideal as you note, but until that is baked into globalize itself

Caching support is ready to land at #470. Basically, it just awaits for more eyeballs to review and make sure it addresses real cases like this one.

@dpolivy

This comment has been minimized.

Show comment
Hide comment
@dpolivy

dpolivy Nov 19, 2015

@rxaviers There are a number of things that we do differently in our modules; for our usage, we require certain functionality that isn't implemented in @devangnegandhi 's solution, but certainly things could be adapted if necessary.

The main thing that we require is the ability to interface with Handlebars, and for that I built handlebars-globalize to go along with my express-globalize module. It wouldn't work as-is against this other library, but could potentially be modified.

A few other things that appear to be different with my implementation:

  • Automatic resolution of locales to bundles (required since we use full lang and country identifiers, e.g. en-US, de-CH)
  • Caching and re-use of globalize objects, and also formatters (needed until the other PR is included)
  • Ability to use without requiring messages
  • Ability to configure which property on the req object contains the desired locale to use (since we populate this via a server database for the user, not a query string, or cookie, etc)
  • Locales can optionally contain a default currency code for that locale, which can simplify currency formatting for default currencies
  • Use of res.locals to store Globalize object, instead of req

I'm not saying my implementation is perfect (feedback is certainly appreciated!), but I can't switch over to the other one as-is.

@devangnegandhi If you want to work on building a unified version that's a hybrid of both of our implementations, let me know. I don't know if you're reviewed mine to see if it might work for your scenarios?

dpolivy commented Nov 19, 2015

@rxaviers There are a number of things that we do differently in our modules; for our usage, we require certain functionality that isn't implemented in @devangnegandhi 's solution, but certainly things could be adapted if necessary.

The main thing that we require is the ability to interface with Handlebars, and for that I built handlebars-globalize to go along with my express-globalize module. It wouldn't work as-is against this other library, but could potentially be modified.

A few other things that appear to be different with my implementation:

  • Automatic resolution of locales to bundles (required since we use full lang and country identifiers, e.g. en-US, de-CH)
  • Caching and re-use of globalize objects, and also formatters (needed until the other PR is included)
  • Ability to use without requiring messages
  • Ability to configure which property on the req object contains the desired locale to use (since we populate this via a server database for the user, not a query string, or cookie, etc)
  • Locales can optionally contain a default currency code for that locale, which can simplify currency formatting for default currencies
  • Use of res.locals to store Globalize object, instead of req

I'm not saying my implementation is perfect (feedback is certainly appreciated!), but I can't switch over to the other one as-is.

@devangnegandhi If you want to work on building a unified version that's a hybrid of both of our implementations, let me know. I don't know if you're reviewed mine to see if it might work for your scenarios?

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Nov 19, 2015

Member

@devangnegandhi could you check the above, please? Thanks @dpolivy

Member

rxaviers commented Nov 19, 2015

@devangnegandhi could you check the above, please? Thanks @dpolivy

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Nov 19, 2015

Member
  • Ability to use without requiring messages

@dpolivy could you please expand on this item? Thanks

Member

rxaviers commented Nov 19, 2015

  • Ability to use without requiring messages

@dpolivy could you please expand on this item? Thanks

@dpolivy

This comment has been minimized.

Show comment
Hide comment
@dpolivy

dpolivy Nov 19, 2015

@rxaviers Disclaimer: I haven't actually tried the module, but from a cursory look at the code it seemed like the messages parameter wasn't optional -- I don't (yet) use that part of Globalize.

dpolivy commented Nov 19, 2015

@rxaviers Disclaimer: I haven't actually tried the module, but from a cursory look at the code it seemed like the messages parameter wasn't optional -- I don't (yet) use that part of Globalize.

@rxaviers

This comment has been minimized.

Show comment
Hide comment
@rxaviers

rxaviers Nov 20, 2015

Member

Ok. Gotcha.

Member

rxaviers commented Nov 20, 2015

Ok. Gotcha.

rxaviers added a commit that referenced this pull request Nov 27, 2015

ashensis pushed a commit to ashensis/globalize that referenced this pull request Mar 17, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment