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

Delivery API no longer compatible with create-react-app? #69

Closed
liamgold opened this issue Aug 25, 2018 · 14 comments
Closed

Delivery API no longer compatible with create-react-app? #69

liamgold opened this issue Aug 25, 2018 · 14 comments
Assignees
Labels

Comments

@liamgold
Copy link

liamgold commented Aug 25, 2018

Since the upgrade of Parse5 from v3 to v5 - 47417f1

create-react-app/webpack is no longer able to create a build when running yarn build:

yarn run v1.7.0
$ react-scripts build
Creating an optimized production build...
Failed to compile.

Failed to minify the code from this file:

        ./node_modules/parse5/lib/utils/mixin.js:3

Read more here: http://bit.ly/2tRViJ9

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

I'm assuming this is expected since the package is now using ES6 rather than ES5?

This also happens in the sample react application: https://github.com/Kentico/cloud-sample-app-react

@liamgold liamgold changed the title Delivery no longer compatible with create-react-app? Delivery API no longer compatible with create-react-app? Aug 25, 2018
@Enngage
Copy link
Contributor

Enngage commented Aug 27, 2018

You are right.. seems like the parse5 does not publish code in ES5. The workaround I was able to get away with is to use browser umd bundle as the main entry point of delivery js sdk. Basically I went to node_modules/kentico-cloud-delivery/package.json and used this as main:

"main": "./_bundles/kentico-cloud-delivery-sdk.browser.umd.min.js"

React build works fine with this. Do you know if there is a better way of aliasing this? Or if react has some sort of build configuration option for this?

@Enngage
Copy link
Contributor

Enngage commented Aug 28, 2018

Imho, this sounds like something that React should address. Also, I found an issue in Parse5 library where the maintainer said that he wants to target modern code rather than ancient ES5 :-)

I would still like to have a better solution for handling this other than what I mentioned above so any comments are welcomed.

@liamgold
Copy link
Author

liamgold commented Aug 28, 2018

Yeah I agree with you - it should be React that resolves it, but it does look like they're recommending putting code on npm as ES5 for at least the next couple years.

On the other hand, they are actively working on the 'next' release of react-scripts, currently "2.0.0-next.a671462c". I've just tested updating my test site to use it, and it seems to be building production code with no problems. I haven't given it a thorough test yet, so not sure how stable it is - but it does look like they've recently updated with security fixes. Looks like it's still 68% way through development, so a while before full release yet.

@petrsvihlik
Copy link
Contributor

@Enngage @liamgold is this something that consumers of this SDK that use it in React projects will regularly be running into? If yes, can it be easily worked around?

@Enngage
Copy link
Contributor

Enngage commented Aug 29, 2018

@petrsvihlik, yes, it causes build to fail with 1.x.y react scripts because react tries to minify it on its own. It would work fine if react enabled developers to disable minification for selected 3rd party libraries. I don't think I'll be able to address this in SDK myself. I would have to take Parse5 dependency, fix it on my own and use that instead.

@liamgold
Copy link
Author

I guess another alternative would be if it's reasonable to just leave the SDK as it is, and just add a warning/disclaimer that it should only be used with react-scripts 2.x.y.

At the same time upgrade the react sample application to use 2.x.y alpha version of react-scripts.

@Enngage
Copy link
Contributor

Enngage commented Aug 29, 2018

@liamgold yeah, I would go down this road as well. I'll likely prepare some sort of FAQ / Common issues page and try to address it there for the time being. Thank you for creating this issue and bringing this to our attention :-)

@petrsvihlik
Copy link
Contributor

@Enngage please do.
wasn't the upgrade to ES6 premature? didn't this change effectively prevent people using react 1.x from building their sites with KC JS SDK?

@Enngage
Copy link
Contributor

Enngage commented Aug 30, 2018

I'm using ES6 + Typescript code from the beginning which I'm compiling to different formats. The problem here is with the parse5 dependency which doesn't compile its code and publishes ES6 straight
to npm. I had to update parse5 because the previous version had different problems as it didn't work in browsers properly (it was initially node.js only lib). Its really difficult right now and I would only wish Delivery API didn't return HTML, but JSON instead.

I wasn't able to find any alternative to parse5 so we have to probably stick with it for now.. I do agree that the react issue is unfortunate, but essentially, react is at faults here because it tries to do the compilation for you even when it shouldn't.

@petrsvihlik
Copy link
Contributor

ok, i get it now, thanks :)

@Enngage
Copy link
Contributor

Enngage commented Sep 4, 2018

FAQ page added and closing for now. I will reference the FAQ from the documentation as well.

@Enngage Enngage closed this as completed Sep 4, 2018
@petrsvihlik
Copy link
Contributor

Was the commit intended to go to the core-separation-cm-api branch?

@Enngage
Copy link
Contributor

Enngage commented Sep 4, 2018

Well, I'll be merging these branches (likely Today) so yeah.. There are quite a few changes in the separation itself so I will create a new branch for CM api only once the master is updated and potentially the v5 released.

@petrsvihlik
Copy link
Contributor

ok, thx

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

No branches or pull requests

3 participants