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

Improve the JS so it supports NodeJS #179

Merged
merged 1 commit into from
Sep 19, 2016
Merged

Improve the JS so it supports NodeJS #179

merged 1 commit into from
Sep 19, 2016

Conversation

bensampaio
Copy link
Contributor

@bensampaio bensampaio commented Sep 14, 2016

I came across this problem when trying to implement server rendering on a ReactJS application dependent on the bazinga-translator. Since the variables window and document don't exist in NodeJS, the Translator initialization would always fail.

To fix that, I improved the module system detection so that when required on NodeJS the code no longer fails because document and window don't exist. Also removed the dependency on the variable undefined.

Besides that, I added uglify-js as a dependency and the build script so other contributors don't need to guess which uglification options should be used. To uglify the JS now you just need to run npm run build under the Resources directory.

What do you think?

…etermine the module system. Add uglifyjs and build script to package json
@bensampaio
Copy link
Contributor Author

I forgot to mention that everything should work as before, the code was just rearranged.

@monteiro
Copy link
Collaborator

@bensampaio it would be good for you to explain the problem you are having at your work as an example in the documentation.

The PR seems OK. I'm going to merge it.

@monteiro monteiro merged commit 3d17193 into willdurand:master Sep 19, 2016
_domains = [];
this.locale = get_current_locale();
if (typeof document !== 'undefined') {
return document.documentElement.lang.replace('-', '_');
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should return en as default locale (or use a different detection strategy). Putting it as null is a bad idea.

/*!
* William DURAND <william.durand1@gmail.com>
* MIT Licensed
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please update the build script so that it keeps the license header

@bensampaio
Copy link
Contributor Author

@stof, sorry for the delay on an answer. Since this was already merged, how do you propose I make the changes? I no longer have the forked repo.

@stof
Copy link
Contributor

stof commented Oct 13, 2016

@bensampaio create a new PR

@pierredup
Copy link
Contributor

This breaks when using RequireJs.
The dumped translations expects the global window variable Translator to be defined, which won't be the case anymore with this change when loading the translation.min.js through RequireJs

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.

4 participants