If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Create node.js bindings

RESOLVED FIXED in 1.0

Status

L20n
JS Library
P2
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: stas, Assigned: stas)

Tracking

unspecified
x86_64
Linux
Dependency tree / graph

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Let's make it so L20n can be used from node.

We might want to have a different set of globals for the server side.
(Assignee)

Updated

5 years ago
Assignee: nobody → stas
Priority: -- → P2
Target Milestone: --- → 1.0
(Assignee)

Comment 1

5 years ago
Most importantly, we need a server side version of io.js.
(In reply to Staś Małolepszy :stas (needinfo along with cc, please; if you want an r+ from me, attach your patch to the bug; I don't review pull requests) from comment #0)
> We might want to have a different set of globals for the server side.

Aaand how much sense does responsive mode have in server-side scenario (same question for python/django and php)
(Assignee)

Comment 3

4 years ago
(In reply to Zbigniew Braniecki [:gandalf] from comment #2)
> Aaand how much sense does responsive mode have in server-side scenario (same
> question for python/django and php)

Globals !== responsive mode.  We could expose static globals too (like @os on the client side).  I don't have any ideas for that at the moment, though.

I also wonder about all the scenarios when node.js app is used as an desktop app.  We could still support @hour and see if we get feedback with other ideas in the future.
(Assignee)

Updated

4 years ago
Blocks: 868093
(Assignee)

Comment 4

4 years ago
Created attachment 745943 [details] [diff] [review]
Patch

Here's a patch that implements IO and the @hour global for node.

I had to do a few file layout changes to make this work, but I really like the outcome.

File layout changes:

 - bindings/ are now outside of lib/
 - platform-specific code should put in modules starting with l20n/platform/
 - node-specific code is in lib/l20n/platform/
 - client-specific code is in lib/client/l20n/platform/
 - platform-specific code includes: io, globals and intl

Code changes:

 - l20n and l20n/platform/io export functions, not a singleton;  this way, you
   can directly do something like:

     var L20n = require('./l20n')
     L20n.getContext()

 - modules that export constructors still need to require them explicitly, e.g.

     var Parser = require('./parser').Parser;

 - l20n/platform/io is consistent in how it rejects promises or throws errors;
   I filed bug 869016 as a follow-up to clean up the error handling code.
Attachment #745943 - Flags: review?(gandalf)
(In reply to Staś Małolepszy :stas (needinfo along with cc, please) from comment #4)
> Created attachment 745943 [details] [diff] [review]
> File layout changes:
> 
>  - bindings/ are now outside of lib/
>  - platform-specific code should put in modules starting with l20n/platform/
>  - node-specific code is in lib/l20n/platform/
>  - client-specific code is in lib/client/l20n/platform/
>  - platform-specific code includes: io, globals and intl

So, why is node code more generic than client code?
Comment on attachment 745943 [details] [diff] [review]
Patch

Review of attachment 745943 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/l20n.js
@@ +5,5 @@
>    'use strict';
>  
>    var Context = require('./l20n/context').Context;
> +  var Parser = require('./l20n/parser').Parser;
> +  var Compiler = require('./l20n/compiler').Compiler;

Why do you load it here?

::: lib/l20n/context.js
@@ +59,1 @@
>          }

Why and what are you trying to catch here?

I believe you want to write code only for IOError, not silencing all possible exceptions.
(Assignee)

Comment 7

4 years ago
(In reply to Zbigniew Braniecki [:gandalf] from comment #5)
> So, why is node code more generic than client code?

Because node implements CommonJS out of the box.

The point of using `define` with the Simplified CommonJS Wrapper, i.e. with the following function signature:

    define(function(require, module, exports) {})

is to be able to write exclusively CommonJS code in the callback.  You don't have to check anywhere in the code if you're running on the server or the client;  it doesn't matter.  Instead, you can configure the module loader on the client side to look for specific modules in non-standard locations.  It's not really possible to do the same for node, because there's no single entry point for code running on the server where you could configure things.  While in lib/ you can require('./l20n/parser'), or you can cd into lib/l20n and require('./parser').  There's nowhere to define a configuration like we can on the clientside by using a module loader (like RequireJS) or by building an optimized version of the library (with Almond or minirequire).


(In reply to Zbigniew Braniecki [:gandalf] from comment #6)
> Comment on attachment 745943 [details] [diff] [review]
> Patch
> 
> Review of attachment 745943 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: lib/l20n.js
> @@ +5,5 @@
> >    'use strict';
> >  
> >    var Context = require('./l20n/context').Context;
> > +  var Parser = require('./l20n/parser').Parser;
> > +  var Compiler = require('./l20n/compiler').Compiler;
> 
> Why do you load it here?

Parser and Compiler are useful for checking the type of an error.  If we remove these lines, two things happen:

 - in the built version of L20n, we'd need to attach Parser and Compiler to L20n in build/browser.js;  you'd then check for errors with `if (e instanceof L20n.Compiler.Error)`.

 - in the dev version of L20n, you'd need to manually do `var Parser = require('./l20n/parser').Parser;`when you want to check `if (e instanceof Parser.Error).

This creates an incompatibility between the code you can run in the built version and the code you can run in the dev version.  It actually bit me a few times already.


> ::: lib/l20n/context.js
> @@ +59,1 @@
> >          }
> 
> Why and what are you trying to catch here?
> 
> I believe you want to write code only for IOError, not silencing all
> possible exceptions.

If you look at l20n/platform/io, the only error it can throw is IOError because all others are caught.  I can check instanceof here; in fact, the first version of the patch did :)  Let me know what your preference is.
Comment on attachment 745943 [details] [diff] [review]
Patch

Review of attachment 745943 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the instanceof IOError in catching.
Attachment #745943 - Flags: review?(gandalf) → review+
(Assignee)

Comment 9

4 years ago
Landed in https://github.com/l20n/l20n.js/commit/8732a653ca38c6a86018b87da54a8bad34691758.

Like I said, the sync vs. async implementation of IO worries me a bit and I'll still want to fix bug 869016 for 1.0.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.