-
Notifications
You must be signed in to change notification settings - Fork 30
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
Add ES6 builds to libraries #3900
Conversation
Tests are passing and the apps are building, so that looks good. Dynamic imports seem to be broken in the app bundles. In the project app:
The content pages app looks good, but it doesn't use internationalisation. EDIT: I forgot to include translations JSON files in the classifier build. Fixed now, and the classifier seems to load. |
307fb3d
to
d438c59
Compare
9c3892b
to
a062247
Compare
a9df545
to
e990b71
Compare
Tests are passing and both NextJS apps are building. I've deployed the project app to kubernetes. It loads and runs without errors, but translated labels don't show for components that are loaded in from EDIT: this was caused by each library calling |
b521da9
to
7440654
Compare
7440654
to
c9c055d
Compare
The project storybook doesn't show translation strings on this branch, which is odd because this PR doesn't touch project translations. EDIT: I think this can be fixed by giving each library its own instance of |
50b3c66
to
4f8d388
Compare
This is working locally for me now without translations bugs, in production mode. The storybooks are working too. I've run a branch deploy of 8f2ba69 so that we can test the Docker image on kubernetes. |
|
|
5760db4
to
c756197
Compare
a7bd75c
to
f065cde
Compare
9c09fba
to
504229f
Compare
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.
This looks great!!
I tested all the linked fe-project-branch
projects, noting changing languages, signing in and out, feedback (positive and negative), d3 related subject viewers - all work as expected. Tested across browsers without issue.
I successfully built and ran app-content-pages and app-project locally. Also ran the storybooks locally; spot-checked a few stories including test translation without issue.
I can sign in/out on the fe-project-branch
linked projects.
Looks great!!
@@ -26,7 +26,8 @@ const supportedLngs = [ | |||
'zh-tw' | |||
] | |||
|
|||
i18n.use(initReactI18next).init({ | |||
const zrcI18n = i18n.createInstance() |
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.
This is terribly minor, 100% ok to dismiss, but with the old Zooniverse-React-Components repo should we avoid referencing the new FEM lib-react-components as zrc
? So maybe this variable could be reactComponentsI18n
or something similar, like how classifierI18n
is the related variable in lib-classifier? I realize reactComponents
has a larger meaning than our new Zooniverse library of react components though, so I understand if would prefer to keep as is.
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.
This library is the replacement for the old ZRC library, in new projects, so I think the naming is OK.
Something I discovered yesterday is that the build I added here (straight Babel transform from JSX to JS) is the build script from the old library. I’m not sure why it’s taken so long to copy this across to the new libraries.
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.
Still kind of confusing that there are two ZRC libraries in NPM.
- Enable the React automatic runtime for the classifier and Zooniverse React Components. - Fix `@babel/runtime` in classifier builds.
- Add an ES6 build to `@zooniverse/react-components`. Export `dist/es6/index.js` as a module. - Add an ES6 build to `@zooniverse/classifier`. Export `dist/es6/components/Classifier/index.js` as a module. - Add `yarn bootstrap:es6` to bootstrap the monorepo with ES modules. - Use `i18n.createInstance()` to set up `i18n` for each library. Give each instance its own `useTranslation` hook and `withTranslation` HOC. - Replace `mime-types` with `mime/lite`. - Remove `@fortawesome/react-fontawesome` and `@fortawesome/fontawesome-svg-core`. - Remove `import * as d3`. - Remove `import _ from lodash`. - Replace `superagent` with `fetch` in the classifier.
504229f
to
ff72487
Compare
b0a2a7b
to
0a1036f
Compare
A quick ADR to explain the switch from Webpack UMD builds to ESM builds with Babel in zooniverse#3900.
A quick ADR to explain the switch from Webpack UMD builds to ESM builds with Babel in zooniverse#3900.
A quick ADR to explain the switch from Webpack UMD builds to ESM builds with Babel in zooniverse#3900.
A quick ADR to explain the switch from Webpack UMD builds to ESM builds with Babel in #3900. --------- Co-authored-by: Delilah C <23665803+goplayoutside3@users.noreply.github.com>
Add ES6 library builds with Babel:
@babel/runtime
in classifier builds.@zooniverse/react-components
. Exportdist/es6/index.js
as a module.@zooniverse/classifier
. Exportdist/es6/components/Classifier/index.js
as a module.yarn bootstrap:es6
to bootstrap the monorepo with ES modules.i18n.createInstance()
to set upi18n
for each library. Give each instance its ownuseTranslation
hook andwithTranslation
HOC.Remove redundant packages from the builds:
mime-types
withmime/lite
.@fortawesome/react-fontawesome
and@fortawesome/fontawesome-svg-core
.import * as d3
.import _ from lodash
.superagent
withfetch
in the classifier.I've left the Webpack UMD module builds in for the Mocha tests, and for backwards compatibility with any third-party projects which
require
in@zooniverse/react-components
. If we can figure out #3779 then I'm fairly sure we can remove the webpack builds and use pure ES modules for the libraries.With the changes here, I'm seeing the JS download on a typical project Classify page drop from ~1.7MB to ~800k. NextJS reports the project page build size as ~490k (Classify pages are ~800k), which is still too large but much better than what we've currently got in production.
Lots of files are touched by this PR, but many of them are one-line changes to the imports from
i18next
packages.Package
app-project
lib-classifier
lib-react-components
How to Review
There are a few options. Pick the one you're most comfortable with:
yarn bootstrap:es6
to skip the Webpack builds, for a faster build that only includes the changes here.I've run a branch deploy of 504229f so that we can test the Docker image on kubernetes.
Here are some test projects:
<head>
of each page, or at the following URL: https://fe-project-branch.preview.zooniverse.org/projects/commit_id.txtChecklist
PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.
General
yarn panic && yarn bootstrap
ordocker-compose up --build
and FEM works as expectedGeneral UX
Example Staging Project: i-fancy-cats
Refactoring