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

create-react-app@2 support #40

Closed
Reinmar opened this issue Oct 2, 2018 · 27 comments
Closed

create-react-app@2 support #40

Reinmar opened this issue Oct 2, 2018 · 27 comments
Assignees
Labels
type:docs type:task This issue reports a chore (non-production change) and other types of "todos".
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Oct 2, 2018

https://reactjs.org/blog/2018/10/01/create-react-app-v2.html

AFAIR one of the things that was supposed to change was support for ES6 dependencies. So perhaps we can test it and improve the integration guide.

@Reinmar Reinmar added type:docs type:task This issue reports a chore (non-production change) and other types of "todos". labels Oct 2, 2018
@Reinmar Reinmar added this to the iteration 21 milestone Oct 2, 2018
@pomek
Copy link
Member

pomek commented Oct 9, 2018

First bug: #41

After reading #41 (comment) I'm not sure whether CRA@2 and #41 are related to each other.

@pomek pomek self-assigned this Oct 9, 2018
@pomek
Copy link
Member

pomek commented Oct 10, 2018

I reported an issue to RCA team because I don't have any idea why it does not work: facebook/create-react-app#5387

@Reinmar
Copy link
Member Author

Reinmar commented Oct 16, 2018

Does #41 happen by default or only when building for production?

I need to somehow warn people about this issue in the blog post and documentation. So I'd like to understand at what stage we should do that.

@pomek
Copy link
Member

pomek commented Oct 16, 2018

Both modes produce an application that does not work :(

@Reinmar
Copy link
Member Author

Reinmar commented Oct 16, 2018

Could you make a PR to the documentation? I think we should add an <info-box warning> pointing people to this ticket. Something like "For compatibility with create-react-app@2 see ...".

Also, I think we need to add @1 to npm i create-react-app so to turn people awareness to that. Unfortunately, it's really unlucky for us that this issue appeared now.

@pomek
Copy link
Member

pomek commented Oct 16, 2018

Could you make a PR to the documentation? I think we should add an pointing people to this ticket. Something like "For compatibility with create-react-app@2 see ...".

ckeditor/ckeditor5#1309

Reinmar added a commit to ckeditor/ckeditor5 that referenced this issue Oct 16, 2018
Docs: Added a warning about lack of support for `create-react-app@2`. See ckeditor/ckeditor5-react#40.
Reinmar added a commit to ckeditor/ckeditor5 that referenced this issue Oct 16, 2018
Docs: Added a warning about lack of support for `create-react-app@2`. See ckeditor/ckeditor5-react#40.
@pomek
Copy link
Member

pomek commented Oct 25, 2018

This issue shouldn't occur anymore after merging babel/babel#8920.

@tayupov
Copy link

tayupov commented Oct 26, 2018

Can confirm. Last week it didn't work, now it does.

@Reinmar
Copy link
Member Author

Reinmar commented Oct 26, 2018

Interesting, because babel/babel#8920 is still open... Are you sure that you haven't changed something else (e.g. switched off transpilation to ES5)?

@tayupov
Copy link

tayupov commented Nov 11, 2018

Sorry, you are right. It might have had to do with me tweaking some configuration.
However I'm back to having the same problem. I'm using CRA2 and didn't eject.

@believe1499
Copy link

I just downgrade the react-script version from 2.1.1 to 1.1.5 , it works

@Reinmar
Copy link
Member Author

Reinmar commented Jan 11, 2019

Unfortunately, we're waiting for babel/babel#8913 to be fixed, but from the discussion, it seems that this is a pretty complex bug so it's hard for us to help there.

@iexplore
Copy link

We are at the point that looking for a good HTML editor in our new application and we like CKEditor 5.
CRA 1 is not an option for us and I would rather not eject.
I saw the custom solution react-app-rewired.
Can this be used and if so, how to configure?

@Reinmar
Copy link
Member Author

Reinmar commented Jan 21, 2019

It seems that react-app-rewired could be a solution, but from its README I read that it's rather dying. Its author mentions other solutions, in fact:

Note: I personally use next.js or Razzle which both support custom Webpack out of the box.

Anyway, you should be able to avoid ejecting your config when using this package. It may not be future-safe, but should work.

However, I started thinking – assuming that you're using a custom CKEditor 5 build (instead of integrating from source) the only reason why you can't use that CKE5 build with CRA2 without ejecting right now is babel/babel#8913. What if we change the source code of CKEditor 5 to avoid this bug? We actually did that in the past already.

@pomek could you look into this? It won't unblock using CKE5 from source in CRA apps, but it will make using builds possible.

@Reinmar
Copy link
Member Author

Reinmar commented Jan 22, 2019

I've just stumbled upon facebook/create-react-app#5387 (comment) by @eugene-kovaljov:

The another possible temporary approach with avoiding the ejection of CRA is to use ClassicEditor as static asset (via script tag in index.html)

My understanding is that it will allow using a custom CKE5 build even in a non-ejected CRA2.

@iexplore
Copy link

If that works it could be a possibilty. The main disadvantage of using it as a static asset is the it can't be lazy-loaded. In our case only most users won't use the editor.

I quess Razzle is also a possibilty although CRA is much more supported.

@pomek
Copy link
Member

pomek commented Jan 22, 2019

@Reinmar, in fact, the fix is really simple.

The described issue touches this part of our code: https://github.com/ckeditor/ckeditor5-utils/blob/5978e4c62580d2691c6689ee17db4fbeba465159/src/emittermixin.js#L240-L248

We can replace it with:

events.forEach( eventName => {
	const destinations = this._delegations.get( eventName );

	if ( !destinations ) {
		this._delegations.set( eventName, new Map( [ [ emitter, nameOrFunction ] ] ) );
	} else {
		destinations.set( emitter, nameOrFunction );
	}
} );

Does the same and works the same as the original code and thanks to that, CRA works without errors.

@Reinmar
Copy link
Member Author

Reinmar commented Jan 23, 2019

Cool! Let's go for it :) Can you make a PR (in ckeditor5-utils as well as in the react integration guide, because we'll not need that warning anymore).

ma2ciek added a commit to ckeditor/ckeditor5-utils that referenced this issue Jan 25, 2019
Other: Replaced `for..of` statement in `EventEmitter` with `Array.prototype.forEach`. This changes allows building a React application using `create-react-app@2`. Closes ckeditor/ckeditor5-react#40.
@ma2ciek
Copy link
Contributor

ma2ciek commented Jan 25, 2019

I've just merged the fix, but note that it won't work with NPM's builds until we release new versions of ckeditor5-* packages (Especially the ckeditor5-utils).

ma2ciek added a commit to ckeditor/ckeditor5 that referenced this issue Jan 25, 2019
@murilocruz
Copy link

I've just merged the fix, but note that it won't work with NPM's builds until we release new versions of ckeditor5-* packages (Especially the ckeditor5-utils).

I couldn't find when are you planning to release the next NPM build. There is any date? I would like to start using it in my production React project, and probably start to contribute with plugins soon!

@ma2ciek
Copy link
Contributor

ma2ciek commented Feb 20, 2019

Hi @murilocruz!

I couldn't find when are you planning to release the next NPM build. There is any date? I would like to start using it in my production React project, and probably start to contribute with plugins soon!

We plan to ship new NPM versions within a week, so stay tuned 🙂

@makis-spy
Copy link

Any updates on this? Hurts to try to use a commercial product and it no workee :)

@luongs3
Copy link

luongs3 commented Feb 27, 2019

Got the same issue. And the only way i can do is to wait

@PaulCombal
Copy link

Hi @murilocruz!

I couldn't find when are you planning to release the next NPM build. There is any date? I would like to start using it in my production React project, and probably start to contribute with plugins soon!

We plan to ship new NPM versions within a week, so stay tuned

2 hours left before I'm officially disappointed! More seriously it looks like a lot of people are waiting for this, can we have any update?

@pomek
Copy link
Member

pomek commented Feb 27, 2019

can we have any update?

Most probably we will release a new version tomorrow.

@pomek
Copy link
Member

pomek commented Feb 28, 2019

We've just released new versions of our builds. Each of them is working with CRA@2.

@pomek pomek pinned this issue Feb 28, 2019
@pomek pomek unpinned this issue Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:docs type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

No branches or pull requests

10 participants