Skip to content
This repository has been archived by the owner on Nov 29, 2023. It is now read-only.

Reference the translator module consistently from widgets and Form.js #410

Merged

Conversation

alxndrsn
Copy link
Collaborator

Make references to translator consistent.

Ideally though, all code would probably refer to fake-translator, and leave this up to 3rd-parties to override with their own translator using e.g. browserify.

@alxndrsn alxndrsn requested a review from MartijnR March 27, 2017 06:37
@MartijnR
Copy link
Member

MartijnR commented Mar 27, 2017

Thanks! Totally right about that error.

Ideally though, all code would probably refer to fake-translator, and leave this up to 3rd-parties to override with their own translator using e.g. browserify.

This is exactly the idea behind require('translator') and having a dummy fake-translator included that doesn't translate anything. Why would calling require('fake-translator') be better?

@MartijnR MartijnR merged commit a08407a into enketo:master Mar 27, 2017
@alxndrsn
Copy link
Collaborator Author

Why would calling require('fake-translator') be better?

There's already a published node module called translator, which is similar but has a different API.

That library doesn't look hugely popular so it's unlikely to ever cause a problem, but I was thinking if the enketo code used a relative require (e.g. require('./fake-translator') then that might be overrideable but also be guaranteed collision-free.

This would be similar to what is done with require( './xpath-evaluator-binding' ); in Form-model.js.

@alxndrsn alxndrsn deleted the consistentize-translator-references branch March 28, 2017 01:46
@MartijnR
Copy link
Member

Thanks. Yes, that's a good reason. The only problem is that it would break things in Enketo-Core consuming apps that are already overriding translator. Maybe good to keep for a 5.x version.

@alxndrsn
Copy link
Collaborator Author

Maybe good to keep for a 5.x version.

If it was changed for translator, then I guess widgets would also be worth changing.

@MartijnR
Copy link
Member

MartijnR commented Feb 14, 2018

As I'm about to make this even worse with a gui module that can overwrite native alert() and confirm() dialogs, how about using something like require('enketo/translator') with this aliasify config:

        "enketo/config": "./config.json",
        "enketo/widgets": "./src/js/widgets.js",
        "enketo/translator": "./src/js/fake-translator",
        "enketo/dialog": "./src/js/fake-dialog",
        "enketo/file-manager": "./src/js/file-manager"

We're avoiding actual relative paths for easy overriding, but are using enketo/ as a namespace basically (no enketo folder has to be present), There is also a scope feature in npm @enketo/translator but I think that may be not be appropriate to (ab)use.

@MartijnR
Copy link
Member

MartijnR commented Feb 14, 2018

Yes, I think I like this. And have changed my mind about needing to wait for a 5.0.0 version because the build system will break immediately which makes it less risky that it is overlooked.

@MartijnR
Copy link
Member

done in 01b6a80

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants