Skip to content

Commit

Permalink
Allow to include objects in translations - this allows to use vdom-no…
Browse files Browse the repository at this point in the history
…des in translations
  • Loading branch information
StephanHoyer committed Jan 29, 2016
1 parent ea29028 commit 71f4732
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 15 deletions.
34 changes: 19 additions & 15 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@
* v0.3.0
*
* Usage:
*
*
* var translate = require('translate.js')
*
*
* var messages = {
* translationKey: 'translationValue'
* }
Expand Down Expand Up @@ -38,23 +38,19 @@
return obj && typeof obj === 'object'
}

function assemble(parts, replacements, count, debug) {
var result = parts[0]
var i = 1
var len = parts.length
while (i < len) {
var part = parts[i]
var val = replacements[part]
function assemble (parts, replacements, count, debug) {
var result = [].concat(parts)

This comment has been minimized.

Copy link
@maranomynet

maranomynet Jan 29, 2016

Collaborator

This adds overhead to the default case.

Also: Array#concat is supposedly quite a bit slower than Array#slice

This comment has been minimized.

Copy link
@StephanHoyer

StephanHoyer Jan 29, 2016

Author Owner

would be great to have actual numbers here :)

This comment has been minimized.

Copy link
@StephanHoyer

StephanHoyer Jan 29, 2016

Author Owner

seems not be the case: airbnb/javascript#243

the performance loss for default cases might be acceptable. Would love to test this though.

This comment has been minimized.

Copy link
@maranomynet

maranomynet Jan 29, 2016

Collaborator

Never ran the numbers myself - It's just something I read somewhere in passing the other day.
Good to know.

for (var i = 1; i < parts.length; i += 2) {

This comment has been minimized.

Copy link
@maranomynet

maranomynet Jan 29, 2016

Collaborator

Classic performance literature says repeated lookups of parts.length are slightly slower than caching the value.
I see no need to stop using the while loop.

This comment has been minimized.

Copy link
@StephanHoyer

StephanHoyer Jan 29, 2016

Author Owner

fixed the length prop.

does it mater if we use while loop or for-loop here? Pretty much the same I think.

This comment has been minimized.

Copy link
@maranomynet

maranomynet Jan 29, 2016

Collaborator

for-loops are fine. :-)

var val = replacements[parts[i]]
if (val == null) {
if (part === 'n' && count != null) {
if (parts[i] === 'n' && count != null) {

This comment has been minimized.

Copy link
@maranomynet

maranomynet Jan 29, 2016

Collaborator

Again repeated lookups are probably slightly slower and also don't minify as well.

This comment has been minimized.

Copy link
@StephanHoyer

StephanHoyer Jan 29, 2016

Author Owner

fixed

val = count
} else {
debug && console.warn('No "' + part + '" in placeholder object:', replacements)
val = '{' + part + '}'
debug && console.warn('No "' + parts[i] + '" in placeholder object:', replacements)
val = '{' + parts[i] + '}'
}
}
result += val + parts[i+1]
i += 2
result[i] = val
}
return result

This comment has been minimized.

Copy link
@maranomynet

maranomynet Jan 29, 2016

Collaborator

Actually... we could place a check here and if the user supplied a custom assembly-result-transformation function when initializing the t() function then feed the result array through it – but if not then default to a simple .join('');

This allows us to ship the library with a minimal (result) => result transformer built-in.

This comment has been minimized.

Copy link
@maranomynet

maranomynet Jan 29, 2016

Collaborator

Like so:

var t = translate(myKeys, {
        transformer: function (parts, replacements) {
            return replacements._myPersonalStringsOnlyFlag ? parts.join('') : parts;
        }
});

and then...

function assemble (parts, replacements, count, debug, transformer) {
  var result = [].concat(parts)
  //
  // do assembly into an array...
  //
  return transformer ? transformer(result, replacements) : result.join('');
}

...

This comment has been minimized.

Copy link
@StephanHoyer

StephanHoyer Jan 29, 2016

Author Owner

but this wouldn't be required than since one could simply do

t = function(a,b,c) {
  return customTransformer(origT(a,b,c))
}

This comment has been minimized.

Copy link
@StephanHoyer

StephanHoyer Jan 29, 2016

Author Owner

I thought of a custom toString method for the translation result.

result.toString = function() {
  return result.join('')
}

but I think this would be a breaking change than

This comment has been minimized.

Copy link
@maranomynet

maranomynet Jan 29, 2016

Collaborator

but this wouldn't be required than since one could simply do
t = function(a,b,c) {
return customTransformer(origT(a,b,c))
}

Hmm...?
It would still be required because origT(a,b,c) would always return a join()ed result string –which makes customTransformer() too late to the game.

This comment has been minimized.

Copy link
@StephanHoyer

StephanHoyer Jan 29, 2016

Author Owner

we always return array than. So basically we support 2 modes, array or string. string mode is default, array-mode allows better post processing.

This comment has been minimized.

Copy link
@maranomynet

maranomynet Jan 29, 2016

Collaborator

ok. :-)

}
Expand Down Expand Up @@ -105,7 +101,7 @@
return result
}

var tFunc = function (translationKey, count, replacements) {
function runTranslation (translationKey, count, replacements) {
var translation = tFunc.keys[translationKey]
var complex = count != null || replacements != null

Expand Down Expand Up @@ -137,6 +133,14 @@
return translation
}

var tFunc = function () {
var translation = runTranslation.apply(null, arguments)
if (translation && translation.join) {
return translation.join('')
}
return translation
}
tFunc.v = runTranslation

This comment has been minimized.

Copy link
@maranomynet

maranomynet Jan 29, 2016

Collaborator

I still have doubts about baking this complexitiy straight into the core of translate.js - as opposed to creating a way to optionally supplant the assemble function.

This comment has been minimized.

Copy link
@maranomynet

maranomynet Jan 29, 2016

Collaborator

Question: Shouldn't runTranslation() auto-join its return Array when in only has Strings in it? Wouldn't that boost performance at the vdom-engine level?

This comment has been minimized.

Copy link
@StephanHoyer

StephanHoyer Jan 29, 2016

Author Owner

I think it doesn't matter who checks if there are only strings in it

This comment has been minimized.

Copy link
@StephanHoyer

StephanHoyer Jan 29, 2016

Author Owner

yeah, maybe it's better to no include it into the lib
let me try that.

This comment has been minimized.

Copy link
@StephanHoyer

StephanHoyer Jan 29, 2016

Author Owner

If I now think closely it would require the user to pretty much copy&paste the original assemble function. Might be not so practical if we update it somehow.

This comment has been minimized.

Copy link
@maranomynet

maranomynet Jan 29, 2016

Collaborator

I still have doubts about baking this complexitiy straight into the core of translate.js - as opposed to creating a way to optionally supplant the assemble function.

In fact I think the custom assemble function approach offers more power and flexibility (allowing more complex/intelligent/weird assembly, etc.) while adding less complexity to the core.

This comment has been minimized.

Copy link
@maranomynet

maranomynet Jan 29, 2016

Collaborator

It means wee need to freeze the assembly signature. ...which shouldn't be a problem.

This comment has been minimized.

Copy link
@StephanHoyer

StephanHoyer Jan 29, 2016

Author Owner

sounds doable. will investigate this at the weekend. Thanks for the comments though

This comment has been minimized.

Copy link
@StephanHoyer

StephanHoyer Jan 29, 2016

Author Owner

also feel free to try this for yourself :)

This comment has been minimized.

Copy link
@maranomynet
tFunc.keys = messageObject || {}
tFunc.opts = options || {}

Expand Down
9 changes: 9 additions & 0 deletions test.js
Original file line number Diff line number Diff line change
Expand Up @@ -258,3 +258,12 @@ describe('translate.js', function () {
expect(t('test', { x: 'HelloWorld' })).to.equal('HelloWorld')
})
})

describe('vdom awareness', function () {
it('should not kill any objects and return arrays as translations', function () {
var t = translate({
test: 'abc {xyz} def'
})
expect(t.v('test', { xyz: { foo: 'bar' } })).to.eql(['abc ', { foo: 'bar' }, ' def'])
})
})

0 comments on commit 71f4732

Please sign in to comment.