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

Updated react-reconciler #44

Merged
merged 2 commits into from
Mar 24, 2019
Merged

Conversation

MarhyCZ
Copy link
Contributor

@MarhyCZ MarhyCZ commented Mar 23, 2019

Hi,
I have updated react-reconciler library to support newest React.js features like React hooks for writing functional components.
Also with that I updated all the bundling dependencies like babel and webpack to their newest versions.

Have a nice day!

react-reconciler updated to support newest React.js features like React hooks for writing functional components.
Copy link
Owner

@a-ignatov-parc a-ignatov-parc left a comment

Choose a reason for hiding this comment

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

Thank you for your effort but I want to ask for a few changes.

Also, I need to check some issues about suspense support in react-reconciler

@@ -43,21 +42,6 @@ if (isProd) {
plugins.push(...[
new webpack.optimize.ModuleConcatenationPlugin(),
new webpack.optimize.OccurrenceOrderPlugin(),
new UglifyJsPlugin({
Copy link
Owner

Choose a reason for hiding this comment

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

Any reasons to remove minification for production build?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because new version of UglifyJS no longer supports ES6 and actually Webpack minifies code automatically when its in production mode. Hence the
mode: isProd ? 'production' : 'development', 58th line

Copy link
Owner

Choose a reason for hiding this comment

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

Got it. Thank you 👍

@@ -43,21 +42,6 @@ if (isProd) {
plugins.push(...[
new webpack.optimize.ModuleConcatenationPlugin(),
Copy link
Owner

Choose a reason for hiding this comment

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

According to mode documentation production mode already includes ModuleConcatenationPlugin and OccurrenceOrderPlugin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I forgot about that, its the same case as in removing the UglifyJS then. I will remove it then.

package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "tvdml",
"version": "6.2.3",
"version": "6.3.0",
Copy link
Owner

@a-ignatov-parc a-ignatov-parc Mar 24, 2019

Choose a reason for hiding this comment

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

Please rollback version value. Releases are made with CI and they use current value to bump version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course, no problem. will do.

@@ -0,0 +1,5962 @@
{
Copy link
Owner

Choose a reason for hiding this comment

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

Please delete package-lock.json because yarn is used as a default package manager

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, no problem

@a-ignatov-parc
Copy link
Owner

Also, please check the tests. they are failing to run. You can run them locally with npm test command

@MarhyCZ
Copy link
Contributor Author

MarhyCZ commented Mar 24, 2019

Also, please check the tests. they are failing to run. You can run them locally with npm test command

Yeah, I see it, its just probably update from babel-register to @babel/register. I will check it all :-)

@a-ignatov-parc
Copy link
Owner

Checked things regarding suspense. There are small changes to reconciler's config but everything should work except hiding elements which is not allowed in TVML.

Will add these changes after merging this PR 👍

@MarhyCZ
Copy link
Contributor Author

MarhyCZ commented Mar 24, 2019

Ok, I updated webpack config, removed the package-lock and changed npm test script to include @babel/register not babel-register. Everything is passing now.

Thank you for keeping this project going 💪

Copy link
Owner

@a-ignatov-parc a-ignatov-parc left a comment

Choose a reason for hiding this comment

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

Nice! Thank you! I'll publish a new version as soon as I check update to reconciler update

@a-ignatov-parc a-ignatov-parc merged commit a00e082 into a-ignatov-parc:master Mar 24, 2019
@a-ignatov-parc
Copy link
Owner

Published as v6.3.0

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.

2 participants