Fix bug 1473330 - Translate.Next architecture

[adngdb]

Here it is at last! The first concrete step towards a rewritten Translate app.

I am aiming at making this first PR a learning experience for everyone who's going to be involved in this. It thus has a very basic feature (fetching entities from django and showing them), but it has the architecture that I think makes sense for a big project, it has tests, it has type checking, and it has documentation.

For this review, I would thus ask that you read the documentation and comment on it (it should serve as a reference for future contributors, hence I want it to be really good), investigate my technical choices, and investigate my architectural choices. Those are the main things I'd like you all to focus on, but of course, the rest is also important.

I thus nominate for review: @mathjazz, @stasm and @Pike. @jotes, @julen, @MikkCZ, and really anyone else interested, I would love to have your feedback, or full review, as well, if you can find the time.

Note that the new feature is hidden behind a switch, so there should be no risk merging it, as it won't be accessible in stage or prod unless we explicitly turn it on.

Things that are missing and might be added to this PR:

• Fix the build
• Localization with Fluent (will be added with the first feature that has actual text content)
• JavaScript linter / Prettier
• CSS extension (SASS? ) (no need for now, will revisit later)
• django-side tests for the Translate views
• More code comments
• Integrate with docker

[julen]

Thanks for the ping @adngdb — I'm currently busy with other stuff and won't be able to provide feedback immediately but would love to check the details and comment on them as I find some spare time.

[Pike]

Thanks for sharing this.

A couple of questions, mostly around onboarding impact and developer productivity:

One is about the use of flowscript. I see some value in typing, but does it outweight the cost? My untrained eye sees a lot of sigils and syntax that's flow-specific, and that anybody wanting to contribute to pontoon frontend would need to understand.

I confess that there's a couple of places where I just don't know what the code is doing, https://github.com/adngdb/pontoon/blob/translate.next/frontend/src/modules/entitieslist/actions.js is one example. There's so much flow in there that I don't find the js with untrained eyes.

There's also quite a bit of set-up to do for https://flow.org/en/docs/editors/. I guess that flowscript without editor support is actually hurting more than it helps? 'Cause you'll be in syntax error land in many places?

For eslint, we'd need eslint-plugin-flowtype ?

How much of flow do we get if we'd not use explicit type annotations?

Another question I have is about https://github.com/adngdb/pontoon/blob/translate.next/frontend/README.md#enable-websocket-and-warm-reloading: You mention a work-around for Firefox. Does that mean that it doesn't work in Chrome or Edge? Would there be ways around this eventually? Like django channels or so? Also, do we need websockets just in development, or also in production?

Last question is about using yarn as test runner. Do you have plans to integrate that back into the django test runners, or would we be left with two entry points to testing?

[jotes]

Hey, I'll need a few moment (days) to dig into details but this PR gives me positive feelings, because of effort you put into documentation and communication (not in the code itself but in other places).

The static typing and test coverage in the front-end are steps in the good direction and I eagerly look into the future.

[adngdb]

One is about the use of flowscript. I see some value in typing, but does it outweight the cost? My untrained eye sees a lot of sigils and syntax that's flow-specific, and that anybody wanting to contribute to pontoon frontend would need to understand.

I agree with that, it does add a lot of code and makes things a lot more complex. I do however believe it is in the end not so different to writing C++ or Rust, and we'll get used to it. However, if we can take the time to learn Flow, that won't be the case for every contributor, so that argument is a good one.

Embracing Flow has a lot of benefits for big applications, it takes care of detecting a lot of bugs before you can even run into them in production. The addons team is using it for the new addons website, and @kumar303 recommended using it.

I see 2 questions thus: do we value code safety over code complexity? Is our application going to be big enough to justify using Flow?

@kumar303, did your team think about code complexity and contributions? If so, what was your conclusion?

There's also quite a bit of set-up to do for https://flow.org/en/docs/editors/. I guess that flowscript without editor support is actually hurting more than it helps? 'Cause you'll be in syntax error land in many places?

Ah, there's actually a command to run flow in a terminal and have it revalidate for each change. I personally haven't integrated it to my editor, I think that would be up to developers and their preferences. The good thing, at least in Atom, is that it doesn't show any syntax errors with Flow. I think the linting plugins I use do support Flow out of the box.

For eslint, we'd need eslint-plugin-flowtype ?

Dunno, but given the name, I guess we will. :P

How much of flow do we get if we'd not use explicit type annotations?

Close to none, if I understand it correctly. Flow can infer types in some cases, but that's quite limited.

Another question I have is about https://github.com/adngdb/pontoon/blob/translate.next/frontend/README.md#enable-websocket-and-warm-reloading: You mention a work-around for Firefox. Does that mean that it doesn't work in Chrome or Edge? Would there be ways around this eventually? Like django channels or so? Also, do we need websockets just in development, or also in production?

Yes, it does mean it won't work in Chrome, I don't know about Edge. I know of no way around that in Chrome, the feature simply doesn't exist there. We can probably work around it in django, but that would be quite complex (at least according to the blog post I used to set this up).

If you can't turn on websockets, you will see errors in the console (that's not very impacting) and you'll have to reload your django server regularly, because polling requests don't close, and after so many web page reloads, the django process won't be able to accept new requests. It's nothing blocking, but it's painful.

Websocket is only useful in development, by react hot reloader to, well, reload the page when there are changes to the files.

Last question is about using yarn as test runner. Do you have plans to integrate that back into the django test runners, or would we be left with two entry points to testing?

Good question! I've never thought about doing that. Right now we actually have 3 different test suites: django, tests for Ryan's JS code, and tests for Translate.Next. I don't think we can run JS tests with pytest, but we have, for docker, a make command to run all tests, make test.

[Pike]

I also wonder about the need to restart the server when switching waffle flags, both Adrian and I would prefer that to "just work". Adrian said he'd look, if I left a comment.

[kumar303]

The addons team is using it for the new addons website, and @kumar303 recommended using it.

Yes, after using it across our team (and contributors) for quite a while now, I still think static typing is very valuable for us. Here are some things that we benefit from on AMO (your mileage may vary):

• Flow will tell us when components are used incorrectly. This is hard to achieve with a test framework unless you are purely doing integration tests which are are slow. This feature makes refactoring very fast. When refactoring is fast and safe, developers are not afraid to improve the architecture as an application scales. Since we use a lot of higher order components, we needed to add type hints to exported components (explained here) to fully achieve it.
• Static typing let us safely write fast and effective unit tests (tests of components in isolation). This is something you cannot do otherwise in a dynamic language. You either end up with unit tests that bit rot over time or you end up with write slow integration tests or you have to practice extreme grep diligence when making changes (something that's very very hard for inexperienced developers/contributors). I wrote about the effective unit test architecture we were able to achieve with static typing.
• Once you learn the syntax, it's easier to read statically typed code and know what's happening and where data is coming from. This is especially helpful for code that interacts with an API. Whenever a developer sits down to write code against an API for the first time, they already have to figure out all the details of each piece of API data. When you use static types, you pass on all of your gained knowledge to the next person who reads the code.

Some additional things I like personally:

• It helps me think about the code I want to write before I write it
• It liberates me to move a little bit faster because I can trust the type safety of other parts of the code I am about to interact with. This is really powerful for new developers who do not have the patterns and idioms of the code base in the back of their mind.

@kumar303, did your team think about code complexity and contributions? If so, what was your conclusion?

Yes, definitely.

Believe it or not, Flow actually helps contributors! This is something I experienced on web-ext which has more contributors than AMO. The problem is that many contributors are not experts in JavaScript so they get caught in many pitfalls. If they use null or undefined incorrectly, they will introduce subtle bugs often without seeing any errors in a console. If they pass arguments to a function incorrectly, JavaScript does not give them any error. The code may or may not work. It probably won't work and they'll see something confusing in the console like 'cannot call method of undefined.' These issues are hard to track down.

With Flow, the contributor receives specific feedback about what they are doing wrong. Some messages are easier to decipher than others but it is definitely easier than trying to write JavaScript when you are not an expert.

Static typing does introduce code complexity. This is something worth considering. I would like to see an improvement to how actions/reducers are typed, maybe like this idea (I can't get it to work, heh). One nice thing, though, is you can introduce types gradually. This might let you focus on important parts of the app first, such as the part that interacts with the API.

Flow still has bugs in it and they can eat a lot of your time. It's better than it was, though. You can use FLOW_FIXME comments to workaround them.

[adngdb]

Once again, thank you very very much for your excellent feedback @kumar303! This is very helpful.

@Pike I think those are sound arguments in favor of using Flow. There's going to be a learning curve, but it will benefit everyone in the end.

[adngdb]

I also wonder about the need to restart the server when switching waffle flags, both Adrian and I would prefer that to "just work". Adrian said he'd look, if I left a comment.

After investigating a bit, I suspect it is the nature of runserver that causes the issue. On a web server that uses wsgi, and a common cache, I believe the cache clearing mechanism of waffle should work correctly. I'd to postpone working on this problem, wait until we are ready to deploy this to stage and test it again there. If that issue only exists for local dev, it will be a lot less problematic.

[adngdb]

I am done with the remaining of the work I wanted to do here. Review away, folks! 🤘

[mathjazz]

Note for reviewers: If you try to run this branch, you'll see make build failing with:

OSError: [Errno 2] No such file or directory: '/app/frontend/build/static'

As @adngdb suggested, run these commands (outside docker) before running make build:

cd frontend
npm install
npm run build

Also note that static files currently don't load unless you turn on the feature:

./manage.py waffle_switch translate_next on --create

[mathjazz]

I'm far from done with the review, so consider this an interim report of the most significant developments that I've encountered with this PR so far:

I got the new laptop!

Later on I set up Pontoon on it via Docker and submitted a patch!
I'm now running this branch and I can confirm that /translate loads some strings.
Kindly allow me to take some more time and dig deeper into this whole new World for me.
Enough is enough, so I'll stop here and continue with the review.

[mathjazz]

I like that you put code documentation in here!

For the sake of consistency, could you please also (as part of this PR, otherwise we'll forget) migrate existing code documentation from RTD?

http://mozilla-pontoon.readthedocs.io/en/latest/dev/sync.html -> /pontoon/api/README.md
http://mozilla-pontoon.readthedocs.io/en/latest/dev/api.html -> /pontoon/api/README.md

[mathjazz]

These links don't seem to render:
https://github.com/adngdb/pontoon/blob/translate.next/frontend/README.md

[mathjazz]

Capital D in django.

[mathjazz]

That's already covered on top of the page. Remove?

[adngdb]

These point directly to the documentation of the tools, I think it's not a problem to have them kind of doubled because I expect it will be convenient at least to me in the future. :)

[mathjazz]

I think this is going to be great. Actually, it already is great!

The docs, comments and commit messages helped me a lot in understanding how the code works and also dig into technologies I haven't used before (still have some work to do here). Thank you!

My only question is this: now we have "React code" in /frontend and /static folders (tags). Should we merge?

Other than that, I think you still need to fix the setup and then we're good to go: #989 (comment).

[mathjazz]

DEV?

[adngdb]

I'm a bit confused about that. On Socorro I only ever used DEBUG. What's the difference between the two?

[mathjazz]

I see that we have DEV and DEBUG both set to False in prod and stage, and both set to True in local development settings. So it might be a good idea to join them.

[mathjazz]

Nit: add space.

[mathjazz]

Remove debug message?

[mathjazz]

Remove debug message?

[adngdb]

My only question is this: now we have "React code" in /frontend and /static folders (tags). Should we merge?

The technology stacks are quite different, merging them might (or might not) be difficult. I'd rather not do that for now and focus on Translate.Next.

[stasm]

Wonderful! Thanks for spearheading this effort; it looks great! And thanks for writing the docs, they helped me review this PR a lot!

I haven't used Flow in production yet, but I'm looking forward to learning more about it. I'm convinced that any type checking technology will benefit Pontoon in the long run.

I like the directory layout proposed for frontend/. One suggestion I might make is to use the .mjs extension for all new code. I expect that it will become the extension for all JS in the future; using it now will spare us the trouble of The Great Rename later on.

I'm yet to try this PR locally because I have run into some trouble with Docker on Windows. I'll give it another try tomorrow. I also haven't looked at the Python nor the CSS parts of this PR.

[stasm]

Maybe just remove it for now, if it's not used anyways?

[stasm]

AFAIK, each RUN creates a new layer in the image. I think the good practice is to use && to execute more than one command when they are related to each other.

[stasm]

👍

[stasm]

componentWillMount will be deprecated in a future React 16.x version and removed in React 17. See https://reactjs.org/blog/2018/03/29/react-v-16-3.html#component-lifecycle-changes. It's best to stick to componentDidMount for side-effects. The reason, in short, is that with async rendering and React Fiber, components may start mounting more than once if they are interrupted by a higher-priority task.

[stasm]

Why are these exported as default rather than as named exports?

[adngdb]

Can you give a code example?

With this I can import the module by writing import navigation from 'core/navigation'; and then use navigation.reducer for example. If we exposed everything as named export, we would have to do import * as navigation from 'core/navigation'; or something, right?

I'm honestly not sure what's best. I went with this because that was the syntax being used in the reference I read: https://jaysoo.ca/2016/02/28/applying-code-organization-rules-to-concrete-redux-code/

[stasm]

I'd recommend the following export structure:

export {default as reducer} from "./reducer";
export const NAME = "...";
// Or, if they're stored in constants.js:
export * from "./constants";

Constants are already uppercase and shouldn't conflict with other exports from index. Flatter hierarchies are quicker to work with :)

Using this elsewhere would look like the following:

// Same as you wrote:
import * as navigation from "core/navigation";

// Or using a rename:
import {reducer as navigationReducer} from "core/navigation";

[adngdb]

Looks good to me. Done!

[stasm]

Can we start using async/await?

[adngdb]

[stasm]

Nit: you don't have to bind the combined reducere to const rootReducer if you only export default it right after. export default combineReducers(...) will work too. (This applies to store.js as well.)

[stasm]

Perhaps change the wording to make this a bit more clear:

... It's recommended to define a NAME constant here which should be a unique identifier of the module. This name will be used in combineReducers and in all mapStateToProps to identify the subtree of the global state relevant to this module.

[stasm]

Also, having a separate file when there's only one constant might be a bit of an overkill. There two modules in this PS and both only have NAME defined. I'd put it directly in index.js and then figure out the good practice for defining more once they are actually needed.

[stasm]

favicon.ico is missing from the this PR.

[stasm]

What is the purpose of exposing Action and State as part of the public interface of the module?

[adngdb]

Good catch, that's some leftover from when I was struggling with Flow.

[adngdb]

@stasm I got started on renaming everything to .mjs but sadly Jest doesn't support .mjs files yet. 😢

[stasm]

@stasm I got started on renaming everything to .mjs but sadly Jest doesn't support .mjs files yet. 😢

Tant pis. I don't think it's crucial to use mjs now. I just thought it was a good opportunity to do so. Thanks for checking!

[mathjazz]

Now I get this error after make run, when I try to load a page:

Compiled successfully!

You can now view frontend in the browser.

  Local:            http://localhost:3000/
  On Your Network:  http://172.18.0.3:3000/

Note that the development build is not optimized.
To create a production build, use yarn build.

[ERROR:django.request] 2018-07-17 12:55:45,358 Internal Server Error: /sl/
Traceback (most recent call last):
  File "/usr/local/lib/python2.7/site-packages/django/core/handlers/exception.py", line 41, in inner
    response = get_response(request)
  File "/usr/local/lib/python2.7/site-packages/django/core/handlers/base.py", line 249, in _legacy_get_response
    response = self._get_response(request)
  File "/usr/local/lib/python2.7/site-packages/django/core/handlers/base.py", line 187, in _get_response
    response = self.process_exception_by_middleware(e, request)
  File "/usr/local/lib/python2.7/site-packages/django/core/handlers/base.py", line 185, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/app/pontoon/teams/views.py", line 42, in team
    'locale': locale,
  File "/usr/local/lib/python2.7/site-packages/django/shortcuts.py", line 30, in render
    content = loader.render_to_string(template_name, context, request, using=using)
  File "/usr/local/lib/python2.7/site-packages/django/template/loader.py", line 68, in render_to_string
    return template.render(context, request)
  File "/usr/local/lib/python2.7/site-packages/django_jinja/backend.py", line 106, in render
    return mark_safe(self.template.render(context))
  File "/usr/local/lib/python2.7/site-packages/jinja2/environment.py", line 989, in render
    return self.environment.handle_exception(exc_info, True)
  File "/usr/local/lib/python2.7/site-packages/jinja2/environment.py", line 754, in handle_exception
    reraise(exc_type, exc_value, tb)
  File "/app/pontoon/teams/templates/teams/team.html", line 3, in top-level template code
    {% import "widgets/menu.html" as Menu %}
  File "/app/pontoon/base/templates/base.html", line 17, in top-level template code
    {% block site_css %}
  File "/app/pontoon/base/templates/base.html", line 25, in block "site_css"
    {% stylesheet 'base' %}
  File "/usr/local/lib/python2.7/site-packages/pipeline/templatetags/ext.py", line 37, in package_css
    return self.render_compressed(package, 'css')
  File "/usr/local/lib/python2.7/site-packages/pipeline/templatetags/pipeline.py", line 56, in render_compressed
    default_collector.collect(self.request)
  File "/usr/local/lib/python2.7/site-packages/pipeline/collector.py", line 38, in collect
    for path, storage in finder.list(['CVS', '.*', '*~']):
  File "/usr/local/lib/python2.7/site-packages/django/contrib/staticfiles/finders.py", line 112, in list
    for path in utils.get_files(storage, ignore_patterns):
  File "/usr/local/lib/python2.7/site-packages/django/contrib/staticfiles/utils.py", line 28, in get_files
    directories, files = storage.listdir(location)
  File "/usr/local/lib/python2.7/site-packages/django/core/files/storage.py", line 397, in listdir
    for entry in os.listdir(path):
OSError: [Errno 2] No such file or directory: '/app/frontend/build/static'

[adngdb]

@mathjazz can you please rm -rf frontend/build then make build && make run? I think this is because you already had a frontend/build/static folder locally, which prevented docker to create the correct links.

[mathjazz]

That folder doesn't exist.

I also noticed this after make run, which might be related:

Leopold:pontoon mathjazz$ make run
/usr/local/bin/docker-compose run --rm --service-ports webapp
Starting pontoon_postgresql_1 ... done
ln: failed to create symbolic link ‘/app/assets/assets’: File exists

[mathjazz]

I also noticed this after make run, which might be related:

No, that happens on master, too (which works apart from that).

[adngdb]

@mathjazz that last commit should fix your issue.

[mathjazz]

Es funktioniert!

[mathjazz]

[adngdb]

Thanks everyone for the reviews and comments! For those who did not have a chance to do it yet, please do feel free to do it anytime, even in this closed PR. I can address your comments in follow-up pull requests, and it's important for me that we do this correctly, so any additional pair of eyes looking at it is welcome.