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

update i18n doc for .travis.yml #710

Closed

Conversation

JasonYCHuang
Copy link
Contributor

@JasonYCHuang JasonYCHuang commented Feb 9, 2017

This change is Reviewable

@coveralls
Copy link

coveralls commented Feb 9, 2017

Coverage Status

Coverage remained the same at 99.329% when pulling 39923b3 on JasonYCHuang:i18n-readme-update into 75cf810 on shakacode:master.

@justin808
Copy link
Member

See comment.


Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


docs/basics/i18n.md, line 92 at r1 (raw file):

If the test are executed in Travis CI, make sure that rake is executed before npm run build in .travis.yml. npm run build depends on translations.js & default.js, and they are generated or updated by rake or rails server.

  1. Not everybody uses Travis for CI
  2. When tests run, they ensure that the the timestamp on the generated files are newer than the source files, or else the build script runs.
  3. We should probably check the i18n files right before building the webpack files. This would cover the case for CI as well, so we would not need this.
  4. In the case of the Rails server, it's fine to ensure that the i18n is built before the server starts, and the people just have to know to restart the server to pick up changes. Otherwise, we might want a specific rake task that can runReactOnRails::LocalesToJs.new

The bottom line is that this small comment, right after the clearly numbered sections, is a bit confusing. We should "just make this work".


Comments from Reviewable

@JasonYCHuang
Copy link
Contributor Author

Agree with your comment.

The best way is to make sure i18n files are generated before building the webpack files.
I18n files are generated by rails, and I have no idea how to execute rake files from javascript files.
Could you give me some hint?


Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

@justin808
Copy link
Member

justin808 commented Feb 11, 2017

@JasonYCHuang Here is where we configure the test helper to check webpack files are ready:

The best way to ensure that the localization files are built before we build the webpack files is to have the npm scripts that build the webpack files first call some rake task. If you depend on the environment, that will probably run the code you put at server startup. However, I'm not sure that we always want the code at server startup. You'd know better.

Here's a rake task that just depends on server startup info:

desc "Pick a random user as the winner"
task :winner => :environment do
  puts "Yes -- we did something. Describe it!"
end

http://stackoverflow.com/q/7044714/1009332

Then change the task to build to first have the call to create the i18n files.

    "build:production": "npm run build:production:client && npm run build:production:server",

https://github.com/shakacode/react-webpack-rails-tutorial/blob/master/client/package.json#L37

@justin808
Copy link
Member

@JasonYCHuang Should we close this PR? Per #717

@JasonYCHuang
Copy link
Contributor Author

@justin808 Yes, please close this PR.

@justin808 justin808 closed this Feb 19, 2017
@JasonYCHuang JasonYCHuang deleted the i18n-readme-update branch March 6, 2017 17:03
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.

3 participants