-
Notifications
You must be signed in to change notification settings - Fork 51
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
Support extraction of $gettext from vue <script> tags #34
Conversation
The goal of this commit is to introduce the possibility to extract localized strings located in Vue.js <script/> tags. This is a first step. For the moment, only strings marked as translatable using the $gettext function are extracted. How to test: - Setup a .vue file like: <template> <h1>{{ greeting_message }}</h1> </template> <script> export default { name: "greetings", computed: { greeting_message() { return this.$gettext("Hello there!") }, duplicated_greeting_message() { return this.$gettext("Hello there!") } } } </script> - Run compile-cli.js with this file Expected result -> Strings present in the script part of the vue component have been successfully extracted.
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.
very few changes but thank you so much for this PR :) I'd also LOVE to have this documented in the README so the users know how to use and configure it (change DEFAULT_VUE_GETTEXT_FUNCTIONS, from instance ?)
@@ -0,0 +1,35 @@ | |||
const Pofile = require('pofile'); | |||
const { MARKER_NO_CONTEXT } = require('./constants.js'); |
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'm not sure we really want to switch to those non-standards indents, do we ? :) unless you change them withibn the whole codebase ? not sure either !
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.
Sorry, this is a matter of habits, I will conform to your standards
|
||
poItem.msgid = this.text; | ||
poItem.references = [ this.reference.toString(withLineNumbers) ]; | ||
poItem.msgstr = []; |
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.
same here ;) please be consistent with the current codestyle, maybe ? did you eslint-check your PR ? we do have an .eslintrc !
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.
Yep, I have ESLint checking the code in background, but It seems the config does not have such a rule to prevent this code format
src/javascript-extract.js
Outdated
@@ -0,0 +1,51 @@ | |||
const acorn = require('acorn'); |
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.
ahem, not sure I've seen acorn pop up in the package.json, does it ? You should import it in the dependencies !
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.
@vperron, I introduced the acorn
package during the last PR from me that you merged. And it's now part of the dependencies in package.json
.
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.
acorn@^5.5.3 was already in the dependencies, AFAIK it is used to parse Javascript expressions in tag attributes :)
|
||
module.exports = { | ||
getNodeTranslationInfo, | ||
}; |
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'm not entirely comfortable with the name of this file :) why don't you just put these functions in the javascript-extract.js
code and export the getNodeTranslationInfo
function for the tests ?
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.
Why not, but honestly I actually prefer to separate the code in a few services for sake of readability and ease of maintenance. However I'm not comfortable with the naming of this file too (I was not really inspired at the moment I've created it) ;)
What do you think about it? Do you have any suggestions on the way I could name it? In other case I'll merge it with javascript-extract.js
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 tried to think about it and couldn't find any "extremely good" name. To me that was the sign that maybe the file was separated for a reason of "forced modularization", more than "well this looks like a good and unified separation that has a proper name" - hence, my proposal to just merge the files together. At least this way `javascript-extract.js`` is a good name that represents "this file contains everything related to javascript extraction of tokens". Do you agree ?
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 did create another file because getNodeTranslationInfo
's job is to build up and to return an object representation of the string being extracted. I wanted javascript-extract to deal only with the parsing and the search of the gettext tokens. Too me it is important not to end up with a file having too much responsibilities.
However It is another point of view, but It is okay for me ;)
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.
Alright, whatever you please !
Looks good to me :) I'll merge that if you have no further improvements. |
I do not have any further improvement to submit (for the moment) :) |
Thanks a million :) |
Hi, I was just wondering, when do you plan to release the next version of easygettext? My team actually needs this feature for the front-end development of Tuleap. Are you waiting to have more content to publish on npm? Please let me know how I could help :) |
Do I understand this correctly that if this works we can leave out parts (or all?) of the example makefile of vue-gettext? |
@escapedcat Of course, but you will have to wait because only the extraction of singular strings ($gettext) is supported for the moment ;) |
I see, thx for the reply! I'll stick to our |
Hey @gorkat , in our component files we use a helper like this:
With 2.5.0 I get:
Looks like it's trying to parse the |
@escapedcat thank you for the feedback! Indeed, the extractor fails on the $gettext variable thinking it is a function. |
Done, thx a lot! Cool improvment! |
We would like to have the possibility to extract localized strings from the script tags in Vue.js files when we use vue-gettext.
Using xgettext is painful and subject to many restrictions:
<template>
and<script>
and breaks mysteriously anyway.The goal of this commit is to introduce the possibility to extract localized strings located in Vue.js
<script>
tags. This is a first step. For the moment, only strings marked as translatable using the $gettext function are extracted.Extraction support for other functions like $ngettext, $pgettext or $npgettext could come later.
If you have any question or feedback, please let me know!
How to test:
Expected result -> Strings present in the script part of the vue
component have been successfully extracted.