refactor: use creact-react-app/react-scripts instead of .d2 folder #711
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 betweend2-app-scripts test
andreact-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: thesrc/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
d2 app scripts test –watch <filename>
doesn’t work -> works.d2
folderd2-app-scripts test
command is more or less an alias forreact-scripts test
so we'll get all the features out of the boxcli
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 thecra-template-dhis2-app
being added)public
folder (e.g. adding scripts like the CKEditor in the maintenance app)Here are some challenges that have to be considered
src/index.js
init
script be able to update as well?)template.json
of the cra template (which is used to define what's in the finalpackage.json
of the app) doesn't copy all values (see here)Here are some fundamental changes
The app-shell is now just a component
<Shell />
wraps the app in thesrc/index.js
<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:
We could use
react-rewire
to hide the index.js but my reasons for not going that way are:index.js
if that's ever needed (it shouldn't be, hence the comment I added)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 theindex.js
, please revert and try to adapt your app"Open ToDos
spawn
to@dhis2/cli-helpers-engine
TEMPLATE_PATH
env var@TODO
commentsstyled-jsx
(see below)Styled-jsx
The way I see it, we have three alternatives:
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:
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 theapp.js
, not theindex.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
./cli/bin/d2-app-scripts init --cwd ./examples [app-name]
yarn install
in the app-platform's root again, as it's a monorepod2-app-scripts start
yarn workspace [app-name] start
d2-app-scripts build
yarn workspace [app-name] build
d2-app-scripts deploy
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
cra-template-dhis2-app/template/src/index.js
toexamples/[app]/src/index.js
cra-template-dhis2-app/template/public
to `examples/[app]/publicexamples/[app]/node_modules
folder and runyarn install
in the app-platform rootyarn workspace [app] start