-
-
Notifications
You must be signed in to change notification settings - Fork 631
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
[WIP] Add json locale format #1271
Conversation
@ashgaliyev Needs docs, changelog, etc. |
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.
see comments
Reviewed 4 of 5 files at r1, 6 of 6 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ashgaliyev)
lib/react_on_rails/test_helper/ensure_assets_compiled.rb, line 32 at r2 (raw file):
self.class.has_been_run = true ReactOnRails::Locales::ToJs.new
The idea is that when we need to prepare bundles for tests, then we prepare the i18n files. However, if we offer both JSON and JS formats, then this should be an option in the config/initializers/react_on_rails.rb
Thus, we don't need a new rake task.
And we don't have to explain that you can only do either JS or JSON
We should also document that JSON should be faster per this article: https://v8.dev/blog/cost-of-javascript-2019 (definitely mention on the doc)
lib/tasks/assets.rake, line 10 at r2 (raw file):
end else Rake::Task.define_task("assets:precompile" => ["react_on_rails:assets:webpack"])
this needs to be reverted
lib/tasks/locale.rake, line 15 at r2 (raw file):
DESC task locale: :environment do ReactOnRails::Locales::ToJs.new
no new task!
lib/tasks/locale.rake, line 24 at r2 (raw file):
DESC task locale_json: :environment do ReactOnRails::Locales::ToJson.new
update per prior comments
@ashgaliyev we could branch off of tag 11.3.0... Or we can cut a new version 12.0.0 |
bb890fc
to
b983889
Compare
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.
I left some comments. Very close!
Reviewed 1 of 1 files at r3, 5 of 5 files at r4.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ashgaliyev)
CHANGELOG.md, line 38 at r5 (raw file):
* Removal of config.symlink_non_digested_assets_regex as it's no longer needed with rails/webpacker. If any business needs this, we can move the code to a separate gem. * Added support to export locales in JSON format.
- Not a breaking change
- Need to mention the new configuration option and the default
- Need to link to the PR and author.
docs/basics/configuration.md, line 185 at r4 (raw file):
# Possible output formats are js and json config.i18n_output_format = 'js'
I think JSON is probably more efficient, due to the https://v8.dev/blog/cost-of-javascript-2019#json
Need to reference that somewhere.
docs/basics/i18n.md, line 79 at r4 (raw file):
In case if you need pure json files, you can specify the format as follows in `react_on_rails.rb`: ```rb config.i18n_output_format = 'json'
Need to show both options...
should JSON be the default?
lib/tasks/locale.rake, line 15 at r4 (raw file):
DESC task locale: :environment do if ReactOnRails.configuration.i18n_output_format == 'json'
What about JSON?
are we going to be strict about lowercase? Should we?
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.
Hi @ashgaliyev a few more comments to address.
Reviewed 1 of 1 files at r6.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @ashgaliyev)
lib/tasks/assets.rake, line 24 at r6 (raw file):
DESC task compile_environment: :webpack do Rake::Task["assets:environment"].invoke
Why do we need this? we need to run the compile command.
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.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @ashgaliyev)
lib/tasks/assets.rake, line 24 at r6 (raw file):
Previously, justin808 (Justin Gordon) wrote…
Why do we need this? we need to run the compile command.
@ashgaliyev check for odd whitespace at end of lines.
lib/tasks/assets.rake, line 34 at r6 (raw file):
DESC task webpack: :locale do if Rake::Task.task_defined?("webpacker:compile")
we can remove this, as we'll assume using rails/webpacker for v12
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.
@justin808 Please, check the latest changes. I also updated doc related to i18n
Reviewable status: 7 of 12 files reviewed, 10 unresolved discussions (waiting on @ashgaliyev and @justin808)
CHANGELOG.md, line 38 at r5 (raw file):
Previously, justin808 (Justin Gordon) wrote…
- Not a breaking change
- Need to mention the new configuration option and the default
- Need to link to the PR and author.
Done.
docs/basics/configuration.md, line 185 at r4 (raw file):
Previously, justin808 (Justin Gordon) wrote…
I think JSON is probably more efficient, due to the https://v8.dev/blog/cost-of-javascript-2019#json
Need to reference that somewhere.
Added to notes in i18n.md
docs/basics/i18n.md, line 79 at r4 (raw file):
Previously, justin808 (Justin Gordon) wrote…
Need to show both options...
should JSON be the default?
Yeah, I think it would be better if JSON as the default option.
lib/react_on_rails/test_helper/ensure_assets_compiled.rb, line 32 at r2 (raw file):
Previously, justin808 (Justin Gordon) wrote…
The idea is that when we need to prepare bundles for tests, then we prepare the i18n files. However, if we offer both JSON and JS formats, then this should be an option in the config/initializers/react_on_rails.rb
Thus, we don't need a new rake task.
And we don't have to explain that you can only do either JS or JSON
We should also document that JSON should be faster per this article: https://v8.dev/blog/cost-of-javascript-2019 (definitely mention on the doc)
I think this is no longer relevant.
lib/tasks/assets.rake, line 24 at r6 (raw file):
Previously, justin808 (Justin Gordon) wrote…
@ashgaliyev check for odd whitespace at end of lines.
Removed this block
lib/tasks/assets.rake, line 34 at r6 (raw file):
Previously, justin808 (Justin Gordon) wrote…
we can remove this, as we'll assume using rails/webpacker for v12
Done.
lib/tasks/locale.rake, line 15 at r2 (raw file):
Previously, justin808 (Justin Gordon) wrote…
no new task!
ok
lib/tasks/locale.rake, line 24 at r2 (raw file):
Previously, justin808 (Justin Gordon) wrote…
update per prior comments
Done.
lib/tasks/locale.rake, line 15 at r4 (raw file):
Previously, justin808 (Justin Gordon) wrote…
What about JSON?
are we going to be strict about lowercase? Should we?
I'm using .downcase
.
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.
LGTM
Reviewed 5 of 5 files at r7.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ashgaliyev)
CHANGELOG.md, line 20 at r7 (raw file):
* Added support to export locales in JSON format. New option added `i18n_output_format` which allows to specify locales format either `json` or `js`. **`json` format is now the default** [PR 1271](https://github.com/shakacode/react_on_rails/pull/1271) by [ashgaliyev](https://github.com/ashgaliyev).
Needs to go lower. I can fix in followup.
Added support to export locales in JSON format. New option added i18n_output_format which allows specifying locales format either JSON or js. JSON format is now the default, which is a breaking change. Set ```ruby config.i18n_output_format = 'js' ``` to get the old behavior of a JS i18n file.
This change is