-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat: remove rollup application compiler, delegate compilation to the shell #187
Conversation
Deploy preview for dhis2-app-platform canceled. Built with commit f98d664 https://app.netlify.com/sites/dhis2-app-platform/deploys/5ddf99fb1bd8820008dbccde |
I added a babel plugin to inject |
Added usage documentation for static files, CSS, and dependencies https://deploy-preview-187--dhis2-app-platform.netlify.com/#/usage |
@varl @dhis2/team-apps this should be ready for testing and review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me. I tried to understand the full impact of this, but that's very difficult as I'm new to the code. Only thing I saw that I didn't like too much is exporting functions in index.js
files, it's always unclear what's inside an index.js unless you look inside. If they're just used for re-exporting, you won't have to look into them all them time. The compile
function is inside an index.js
, which I def. did not expect. Can we move that one to its own file and just export it from the index.js?
@Mohammer5 Check out the port of Data Vizualizer to the App Platform if you want to get an idea of the full impact: dhis2/data-visualizer-app#327 |
Done. It makes index.js a bit superfluous, and we do have index.js logic elsewhere in this repo, but I can get behind the no logic in index files rule. Thanks for taking a look through the code @Mohammer5 ! |
Uber nice, cheers! |
It should be exported in the import { config } from '@dhis2/cli-style'
const browserslistsrc = config['browserslist']
// path/to/node_modules/@dhis2/cli-style/config/js/browserslist.config.rc Maybe you can use that? |
Yep, that’s what I was thinking! Will do it with another PR though |
# [2.0.0](v1.5.10...v2.0.0) (2019-11-28) ### Features * remove rollup application compiler, delegate compilation to the shell ([#187](#187)) ([cae7a07](cae7a07)) ### BREAKING CHANGES * This may break some applications which use the former named import workaround, but removing that workaround should make treeshaking work!!
🎉 This PR is included in version 2.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Closes #72 |
BREAKING CHANGE: This will break applications which use the named import workaround, but now treeshaking will work!!
This overhauls (actually strips down) the application compiler, delegating most of the compilation and bundling responsibility to Create React App in the shell. This has several benefits:
svg/png
imports just work - as does anything supported by CRAThere are also a few of drawbacks, however:
shell
and manage our own configuration there, which might not be a bad idea.babel
with (currently) only thestyled-jsx
plugin enabled. This means the source files processed by CRA might not be identical to the source files the developer sees in their./src
directory.It also means we run into an uglyeslint
error becausestyled-jsx
generates non-compliant_JSX
components which break the CRA linter.rollup
(excluding dependencies) and build both ES and CJS bundles.pnp
for shell dependencies any more because we need to rely on find-up resolution to get to app-specific dependencies in the top-levelnode_modules
. This increases shell bootstrap/install latency and requires a lot more disk space to bootstrap a local shell.TODO
node_modules
when copyingshell
andadapter
assets intocli
dist bundlepublic
directory for static assetsd2-app-scripts
is upgradedstyled-jsx
generated code.