Skip to content
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

refactor: use creact-react-app/react-scripts instead of .d2 folder #711

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Mohammer5
Copy link
Contributor

@Mohammer5 Mohammer5 commented Mar 30, 2022

Hey guys, I was working on this sort of in secret hehe.. I felt somewhat annoyed by the .d2 folder and the issues that can happen with it as well as the double file watching (ours and the of of cra inside .d2), the discrepancy between d2-app-scripts test and react-scripts test and some minor things.

So I was wondering how we could do things differently if we used create-react-app directly. I haven't changed any of the public api of d2-app-scripts. Everything still works the same (with one difference: the src/index.js and the app-shell; but more on that a bit later).

Here are the reasons why I think this is a good change

  • Eliminates some of the issues mentioned during the "Frontend Initiatives Meeting" on 2022-03-29
    • Like the start script silently exiting
    • d2 app scripts test –watch <filename> doesn’t work -> works
    • "dependencies and .d2 issues" -> no more .d2 folder
    • Linking local libraries should be trivial
  • No more double file watching
  • Relying directly on create-react-app's tools without having to implement certain things ourselves (like file watching, test command, building the app)
  • The d2-app-scripts test command is more or less an alias for react-scripts test so we'll get all the features out of the box
  • Some code from cli was removed, reducing the complexity in that part of the code (note: the reason why there are more lines added than deleted is mostly due to the cra-template-dhis2-app being added)
  • We have direct control over the public folder (e.g. adding scripts like the CKEditor in the maintenance app)
  • Saver transition for the maintenance-app as we can use react17 and react15 in the same app, similarly to what I demo'ed a couple of months ago
  • Dependencies of the shell / adapter become peer dependencies, removing the issue of multiple package versions
  • VSC debug works

Here are some challenges that have to be considered

  • This change introduces a src/index.js
    • IMO this is not a bad change, but actually a good one
    • We'd have to think about how we can let users transition from the old style to new one (should the init script be able to update as well?)
  • The template.json of the cra template (which is used to define what's in the final package.json of the app) doesn't copy all values (see here)

Here are some fundamental changes

The app-shell is now just a component

  • The <Shell /> wraps the app in the src/index.js
  • The two style rules have been moved to the <app-root>/public/index.html

Nudging instead of IoC (with the shell)

Best book on nudging

In theory the user could mess with the index.js, I've added a prominently visible comment that - in essence - says something like "DO NOT TOUCH THIS FILE!".

The reason why I use the term "nudging" is:

  • Users could modify things that are now closed right now
  • It is fairly hard to do that though as you really have to be a dhis2 expert and replicate the shell
  • This will nudge users into going the way we're "prescribing"
  • This is very close to our philosophy of "Trivial things should be easy, complicated things should be possible"

We could use react-rewire to hide the index.js but my reasons for not going that way are:

  • we'd have to mess with cra's internal tooling, we'd introduce a point of failure if they change that
  • it removes the possibility of doing things in the index.js if that's ever needed (it shouldn't be, hence the comment I added)
  • if the dev messes with this file and things stop working, they're most likely going to create a mess anyway. I wouldn't cater to dev beginners, they're not ready to lead app development
  • it'd increase the complexity of our codebase
  • We reject any support when the index.js has been altered unless the person with the issue can explain why it was necessary AND we agree with that assessment, otherwise we'll respond with "Unnecessary change in the index.js, please revert and try to adapt your app"

Open ToDos

  • Add pwa back (I haven't done that yet because I want to understand the code first)
  • Figure out the differences between our approach and the cra pwa approach
  • Add spawn to @dhis2/cli-helpers-engine
  • Document the TEMPLATE_PATH env var
  • Address all remaining @TODO comments
  • Figure out how to address styled-jsx (see below)
  • How to address i18n? e.g. watching, etc
  • Look into dynamic module loading (virtual fs?)
  • https://github.com/dhis2/app-platform/blob/master/cli/config/makeBabelConfig.js
  • Browserlist
  • jest config (jsx? ts?)
  • pwa? Keep it the way it is?

Styled-jsx

The way I see it, we have three alternatives:

  1. Keep the old way around and refactor it so it uses the new shell as well (easiest solution)
  2. Try to get react-app-rewired to work (I wasn't able to get that working so far; but I'm a webpack noob, so help required)
  3. Release a new major version, forcing our users to refactor

Option 3 would be my preferred choice.

It follows the versioning philosophy, apps with styled-jsx can still use the older version and refactor over time.
The functionality of our cli tool is not extended rapidly, and apps are working today already, so they'd only miss new features.
We can even backport important new features or bugfixes to the older version, which is also viable as there won't be a lot of new features in a short period of time and bugs have occured rarely so far, we can expect that to stay the same.

If we decide that option 3 is off the table, then I'd go for option 1 and deprecate the styled-jsx way.
This way we give users a time to refactor while we can re-use the updated <Shell /> as well as <AppAdapter /> component in the legacy shell code to reduce the maintenance burdern.

Option 2 has the disadvantage that's highlighted in the README of react-app-rewired:

As of Create React App 2.0 this repo is "lightly" maintained mostly by the community at this point.

warning Please Note:

By doing this you're breaking the facebook/create-react-app#99 (comment) that CRA provides. That is to say you now "own" the configs. No support will be provided. Proceed with caution.

"Stuff can break" — Dan Abramov https://twitter.com/dan_abramov/status/1045809734069170176

Global app shell

At first this seems like it's not going to work with a global app shell, but I think we can get it to work fairly easily. Regardless of what approach we'll take, we'll have to create ES Modules from our apps.

I've kept the d2.config.js which points to the app.js, not the index.js. This will allow us to create ES Modules that export the actual app component, completely disregarding the CRA setup, which leaves the CRA setup as a development environment we can leverage on.

Before we transition to the global app shell, we can even leverage on CRA's build tool (implemented in this PR already) to create a functional, for production optimized, build.

How to test this

  • d2-app-scripts init
    • You can create a new example app by running: ./cli/bin/d2-app-scripts init --cwd ./examples [app-name]
    • You'll have to remove the node_modules folder in that project and run yarn install in the app-platform's root again, as it's a monorepo
  • d2-app-scripts start
    • Either run that command in the example app or do it on the root yarn workspace [app-name] start
  • d2-app-scripts build
    • Same as above: yarn workspace [app-name] build
  • d2-app-scripts deploy
    • Same as above, but you need to run the build script before: yarn workspace [app-name] deploy https://debug.dhis2.org/dev

Another way to test this is to copy one of our applications to the examples folder, then

  • copy cra-template-dhis2-app/template/src/index.js to examples/[app]/src/index.js
  • copy cra-template-dhis2-app/template/public to `examples/[app]/public
  • remove the examples/[app]/node_modules folder and run yarn install in the app-platform root
  • then run yarn workspace [app] start

@Mohammer5 Mohammer5 changed the title refactor: use creact-react-app/react-scripts instead of .d2 folder magic refactor: use creact-react-app/react-scripts instead of .d2 folder Apr 1, 2022
@amcgee
Copy link
Member

amcgee commented Apr 21, 2022

Alternative approaches, some of which I mentioned today:

  1. Remove the create-react-app indirection and run our own compile/bundle steps directly on the source files, as we currently do for libraries, so that we don't have to work around the limitations of CRA. We could even reuse some of the CRA internals (a la ejecting)
  2. Build a dynamic module instead and run the shell unmodified, then dynamically load the bundled module
  3. Use a more flexible intermediary like nextjs which provides some of the features we want (including the ability to customize build steps)

An important consideration is security and sandboxing of applications, we need to be able to provide "guarantees" to people installing DHIS2 applications. This leads to the IoC paradigm we chose, which allows us to control and restrict the problem space of applications (while trying to keep that restriction as flexible as possible). We also want to make sure we're not pushing complexity from the platform to apps, we should strongly prefer encompassing complexity in the platform rather than proliferating it in hundreds of applications and branches. The many applications built with our platform

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

Successfully merging this pull request may close these issues.

None yet

2 participants