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

Upgrade frontend libraries #2295

Merged
merged 8 commits into from
May 23, 2017

Conversation

kxmbrian
Copy link
Contributor

  • Make necessary changes to upgrade for React 15.5 and to prepare for React 16.
  • react-summernote has not made the upgrade. Submitted a PR to the repo for that. Right now, it only generates warnings.
  • Following up to Standardise frontend date and time formatting #2278, removed timeZone prop is no longer required as a prop to IntlProvider. Switched to using yahoo/react-intl instead of jeremyyap/react-intl.

@allenwq
Copy link
Member

allenwq commented May 18, 2017

👍 @kxmbrian Does the libraries in the webpack config need to be updated ?

@kxmbrian
Copy link
Contributor Author

Update: react-summernote PR has been merged, but maintainer has not published the latest package.

Latest changes:

Note: you might have to reinstall the node-sass package on your local machine if you get this issue. I did yarn remove and yarn add, but you can try yarn install --force

loadModules();
import(/* webpackChunkName: "intl" */ 'intl');
import(/* webpackChunkName: "intl" */ 'intl/locale-data/jsonp/en');
import(/* webpackChunkName: "intl" */ 'intl/locale-data/jsonp/zh');
Copy link
Member

@allenwq allenwq May 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kxmbrian Is the import here async ? If it is then we will need to load_modules after import success.

From their examples it seems that we might need sth like:

Promise.all([import("intl"), import("intl/xxx")]).then(function() {
    load_modules();
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated!

@kxmbrian kxmbrian force-pushed the kxmbrian/frontend branch 3 times, most recently from be780a7 to 3d21ee4 Compare May 23, 2017 06:26
@allenwq allenwq merged commit 397004d into Coursemology:master May 23, 2017
@kxmbrian
Copy link
Contributor Author

@weiqingtoh Have made another PR to react-summernote to expedite the update, however, even if it were merged, we will still get warnings because the latest build of react-summernote uses Promises but phantomjs does not support Promises natively.

It is possible to polyfill it for the time being, but it might be better to swap out phantomjs instead #2270. Shall we leave it for now and use the production build instead of the test build when testing locally?

Related to #2307 (comment).

@weiqingtoh
Copy link
Member

@kxmbrian My main concern is whether or not we are able to catch critical JS errors using the production build - because it does ignore some js errors after all. Would it be possible to study that?

The move to chrome headless might have to wait a little cause it still requires for us to manually build chrome locally, and I'm not too keen to impose that on the development team yet. If the polyfill does not take a significant amount of time, I think it's all right to do that first.

@kxmbrian
Copy link
Contributor Author

Yup, I agree that we should let all JS warnings come to light and we should wait a while before moving to headless chrome.

Made a PR for the polyfill #2309

@weiqingtoh
Copy link
Member

👍 Thanks @kxmbrian !

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

Successfully merging this pull request may close these issues.

None yet

3 participants