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

React 16 support #242

Closed
wants to merge 15 commits into from
Closed

React 16 support #242

wants to merge 15 commits into from

Conversation

mikecousins
Copy link
Contributor

@mikecousins mikecousins commented Jul 27, 2017

This should cover everything from #191. I think I've fixed all of the remaining things: moving it to dependencies, marking it external and fixing the react dom factories issue.


@alexkrolick
Copy link
Collaborator

Looks great! I'll test it out later.

One thing though - do you think you can do the formatting changes separately for the sake of the Git history?

@mikecousins
Copy link
Contributor Author

Okay, I removed my readme changes for now. I also added support for React 16 beta.

webpack.dev.js Outdated
@@ -39,7 +39,13 @@ module.exports = {
'commonjs2': 'react-dom/server',
'amd': 'react-dom/server',
'root': 'ReactDOMServer'
}
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

This block seems misaligned relative to its neighbors - tabs v spaces issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it seems like that file is using tabs for some reason?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's just stick with whatever is there for now, I'll run the linter over everything later. Same with the whitespace changes in the component.

Copy link
Owner

@zenoamaro zenoamaro Jul 28, 2017

Choose a reason for hiding this comment

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

Library was indeed authored with tabs. I dropped that battle eventually, but this is indeed not the right time to diverge. It will be dealt with later in one shot.

package.json Outdated
@@ -49,7 +49,7 @@
"react-dom-factories": "^1.0.0"
},
"peerDependencies": {
"react": "^0.14.9 || ^15.3.0 || ^16.0.0"
"react": "^0.14.9 || ^15.3.0 || ^16.0.0-beta"
Copy link
Owner

Choose a reason for hiding this comment

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

I'm torn on this. I don't think it's a big deal if you get peer warnings for betas.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree, even if it is technically true, it doesn't make sense for package consumers to mark compatibility with unreleased versions with unstable APIs.

package.json Outdated
@@ -44,25 +44,27 @@
],
"dependencies": {
"lodash": "^4.17.4",
"quill": "^1.2.6"
"prop-types": "^15.5.10",
"quill": "^1.3.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a hard requirement? Technically I don't see any 1.3.0-specific APIs being used.

@mikecousins
Copy link
Contributor Author

Let me know what you think now. I think I've made all of the changes requested.

@mouhsinelonly
Copy link

will this be merged soon, because we want to upgrade to react 16

@alexkrolick
Copy link
Collaborator

alexkrolick commented Aug 1, 2017

Thanks @mikecousins! Will do some manual testing to confirm everything works and then merge if 🍏

@alexkrolick
Copy link
Collaborator

alexkrolick commented Aug 2, 2017

@mikecousins I'm getting the "'ReactQuill not found. Did you run "make build"?'" popup when I try to open demo/index.html from your fork. Are you seeing that as well or can you run the demo?

@mikecousins
Copy link
Contributor Author

mikecousins commented Aug 2, 2017

I don't have make, so I can't run the build. Where do I get make from?

Is it this? http://gnuwin32.sourceforge.net/packages/make.htm

Edit: nevermind I installed this and am trying it out now (https://www.npmjs.com/package/make)

@mikecousins
Copy link
Contributor Author

mikecousins commented Aug 2, 2017

PS C:\GitHub\react-quill> npm run build

> react-quill@1.0.0 build C:\GitHub\react-quill
> make build

make i info Invoking build target
'$' is not recognized as an internal or external command,
operable program or batch file.
make × ERR  @$(WEBPACK) --config webpack.dev.js
@$(WEBPACK) --config webpack.prod.js
@cp node_modules/quill/dist/quill.core.css dist
@cp node_modules/quill/dist/quill.snow.css dist
@cp node_modules/quill/dist/quill.bubble.css dist
@mkdir -p $(LIB)
@cp -Rfv $(SOURCE)/* $(LIB)
make × ERR  Recipe exited with code %d
(node:48152) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: Recipe exited with
code %d
(node:48152) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejection
s that are not handled will terminate the Node.js process with a non-zero exit code.
PS C:\GitHub\react-quill>```

@mikecousins
Copy link
Contributor Author

mikecousins commented Aug 2, 2017

PS C:\GitHub\react-quill> npm run test

> react-quill@1.0.0 test C:\GitHub\react-quill
> make test

make i info Invoking test target
make i info Invoking lint target
'$' is not recognized as an internal or external command,
operable program or batch file.
make × ERR  @$(LINT) $(LINT_FLAGS) $(SOURCE)
make × ERR  Recipe exited with code %d
(node:40428) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: Recipe exited with
code %d
(node:40428) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejection
s that are not handled will terminate the Node.js process with a non-zero exit code.
make i info Build success in 427ms
PS C:\GitHub\react-quill>```

@alexkrolick
Copy link
Collaborator

alexkrolick commented Aug 2, 2017

Yeah, that will probably only work on a Unix-y system, like Bash on Windows/Cygwin/MacOS/Linux.

Really you just need to translate these commands for the build task:

./node_modules/.bin/webpack --config webpack.dev.js # Build
./node_modules/.bin/webpack --config webpack.prod.js # Build
cp node_modules/quill/dist/quill.core.css dist # Copy assets
cp node_modules/quill/dist/quill.snow.css dist # Copy assets
cp node_modules/quill/dist/quill.bubble.css dist # Copy assets
mkdir -p ./lib # Make lib folder
cp -Rfv ./src/* ./lib/ # Copy built files to lib folder

@mikecousins
Copy link
Contributor Author

mike@MIKE-BRIGHTSQUID:/mnt/c/GitHub/react-quill$ npm run build
: not foundram Files/nodejs/npm: 3: /mnt/c/Program Files/nodejs/npm:
: not foundram Files/nodejs/npm: 5: /mnt/c/Program Files/nodejs/npm:
/mnt/c/Program Files/nodejs/npm: 6: /mnt/c/Program Files/nodejs/npm: Syntax error: word unexpected (expecting "in")
mike@MIKE-BRIGHTSQUID:/mnt/c/GitHub/react-quill$ npm run test
: not foundram Files/nodejs/npm: 3: /mnt/c/Program Files/nodejs/npm:
: not foundram Files/nodejs/npm: 5: /mnt/c/Program Files/nodejs/npm:
/mnt/c/Program Files/nodejs/npm: 6: /mnt/c/Program Files/nodejs/npm: Syntax error: word unexpected (expecting "in")
mike@MIKE-BRIGHTSQUID:/mnt/c/GitHub/react-quill$

@mikecousins
Copy link
Contributor Author

I just went down a pretty deep rabbit hole trying to modify the makefile. I'm going to give up for now.

@alexkrolick
Copy link
Collaborator

alexkrolick commented Aug 2, 2017

Did you see the comment above about the webpack commands?

I think on Windows you just need to translate the mkdir and cp commands - could just use File Explorer and copy-paste.

Sorry for the difficulty - we should probably rewrite these actions as cross-platform node scripts.

If you don't have more time I'll try again later on a different computer.

@mikecousins
Copy link
Contributor Author

I manually ran those commands now and I get a React not found popup followed by a ReactQuill not found popup.

@mikecousins
Copy link
Contributor Author

mikecousins commented Aug 2, 2017

Looking into the demo code it's a pretty strange setup that I don't quite follow.

image

Every single file it tries to load fails. Does this demo ever work? I haven't changed anything related to it.

@alexkrolick
Copy link
Collaborator

Yes, it works on current master.

@mikecousins
Copy link
Contributor Author

Well, I have no idea why.

@alexkrolick
Copy link
Collaborator

Here's why: https://github.com/mikecousins/react-quill/pull/2 😄

Since prop-types is an external in webpack we need to add it our demos and codepen examples.

@zenoamaro
Copy link
Owner

Looking into the demo code it's a pretty strange setup that I don't quite follow.

@mikecousins, I believe this might be an issue with symbolic link support on Windows:

screen shot 2017-08-03 at 18 55 53

Have a look at your demo folder, and if you don't have such files, link them or copy them there.

@alexkrolick
Copy link
Collaborator

@zenoamaro see the PR I made into @mikecousins branch that adds a symlink for prop-types.js

https://github.com/mikecousins/react-quill/pull/2

If you want to work on it further I pulled in the changes in this PR onto https://github.com/zenoamaro/react-quill/tree/react-16-compat

@mikecousins
Copy link
Contributor Author

mikecousins commented Aug 3, 2017

Nice work guys. 👍 I'll take a look at the new branch.

@alexkrolick
Copy link
Collaborator

Closing in favor of #253

@alexkrolick alexkrolick closed this Aug 4, 2017
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.

Upgrade Quill to 1.3? New lines added above headings when re-render with the updated text
4 participants