search

Olical / react-faux-dom

DOM like structure that renders to React (unmaintained, archived)
http://oli.me.uk/2015/09/09/d3-within-react-the-right-way/
The Unlicense
1.21k stars 87 forks source link

Error running through UglifyJs #96

Closed tibotiber closed 7 years ago

tibotiber commented 7 years ago

There is a new error due to the HOC. I get it when running yarn build in a create-react-app. Basically UglifyJs fails to process the HOC, apparently expecting it to be already compiled to ES5. Isn't the lib run through babel in a prepublish step? Not sure how to approach this. Any idea @Olical?

react-scripts build 
Creating an optimized production build...
Failed to compile.

static/js/main.2f8d0454.js from UglifyJs
SyntaxError: Unexpected token: operator (>) [./~/react-faux-dom/lib/withFauxDOM.js:4,0]

error Command failed with exit code 1.
Olical commented 7 years ago

The babel step only happens to lib/ReactFauxDOM.js I think. We'd have to have two entry points or require the HOC from within the main file to get it to work. Maybe we should require it and export it, but I'm not sure if it's better having it optional.

We could always just take out ES6+ code.

sdhicks commented 7 years ago

I just came across the same issue described by @tibotiber.

I've found ways to make UglifyJS work but it uses development code that I'm not really comfortable with: https://github.com/webpack/webpack/issues/2545#issuecomment-300492798

@Olical What prevents using dist/ReactFauxDOM.min.js instead of lib/ReactFauxDOM.js in package.json main?

EsrefDurna commented 7 years ago

+1 Uglify throws error, if you having above error do not switch to react-faux-dom 4.0.0 stay at 3.1.0

tibotiber commented 7 years ago

@sdhicks :+1: for the idea. I just opened a PR (#98) doing exactly that and tested locally, it solves the issue. I was unfamiliar with the main config in package.json, thanks for pointing that out.

Olical commented 7 years ago

I'd prefer not to default to a minified version, any function name mangling done in the name of network transfer then has an impact on people using it during development or on the server. All stack traces will be ruined since there are no source maps. So I'm reluctant.

Is the issue just that we have ES6 code that can't run through uglify? I'd say the better approach would be to make sure it is run through babel like the rest of the code, then people can uglify if they want to. I, personally, really don't like consuming minified files during development, I prefer to do the minification myself as a build step.

tibotiber commented 7 years ago

@Olical good point. Should we remove the minifying from the build step then? We could still run the code through babel without minifying? I think the problem does not come from ES6 but from JSX in the code.

Olical commented 7 years ago

It's because you're using an arrow function I think. The rest of the code doesn't even use ES5+ as far as I can remember? I'm not sure why I even have babel in the first place. Would you be able to remove those ES6 features? If not I'll try to do it today, maybe over lunch.

I'd rather steer clear of babel and the mountain of complexity that is transpiled JavaScript. I can't actually remember why it's even referenced anywhere, pretty sure all I used was webpack and uglify to build the minified browser version.

tibotiber commented 7 years ago

Oops, my bad, no JSX indeed.

So, I'm OK to switch to full ES5 but may require some help to do so.

Considering this complication in implementation and the potential performance drawback from not using class components, we might still prefer to use ES6 and generate a non minified but transpiled version for the main? The current babel config seems good and we need to manage it for the browser version anyway. I'm wondering how other libs handle this?

Olical commented 7 years ago

Alright, on my lunch break, taking a look.

sdhicks commented 7 years ago

@tibotiber @Olical I think using ES6 is perfectly fine especially with all the added functionality and performance benefits! It's just the code that is published should be compiled down to ES5. In the future (with the harmony dev branch of uglify) this won't be an issue. This might be a long way off though?

From what I've gathered from a few libs (https://github.com/nfl/react-helmet is the example I'll use here) is they typically have an src/ folder for their own development but published releases contain a lib/ and es/ (less frequent) folder with babel compiled code. Their package.json main entry point is ./lib/Helmet.js.

Let me know if I can do anything to help!

Olical commented 7 years ago

Yep, I know about src -> lib on release, I've done it for a few things at work. I just prefer to remove it where I can, I don't see any reason to use the new syntax other than convenience over large code bases. For libraries I like to keep it simple. I'm converting it to ES5 now. Not sure why ES6+ would be faster though, it's only syntax, not runtime :slightly_smiling_face:

Many developers steer clear of it transpiling entirely, which I can completely understand, so I'd rather spend a little more effort to make it easier and leaner for people downstream.

sdhicks commented 7 years ago

@Olical @tibotiber thanks for the quick responses and fix btw!

Olical commented 7 years ago

Here's my PR that ES5s all the things: https://github.com/Olical/react-faux-dom/pull/100

Thoughts? Does this work? Not sure if anyone has a nice broken environment they can pull this into to check. A real world scenario would be best!

Olical commented 7 years ago

I think I can delete most of the stuff that refers to babel, I don't actually know why it's there. There's some consts but they're in the tests where they're okay anyway.

Olical commented 7 years ago

@sdhicks @tibotiber @EsrefDurna - can any of you confirm that this works?

I accidentally published it as 4.0.1. I tried to publish a next tag but did something wrong :neutral_face: - so I hope it's okay!

sdhicks commented 7 years ago

@Olical checking here shortly

sdhicks commented 7 years ago

@Olical

I didn't notice 4.0.1 on npm so I cloned this repo and checked out your pr's branch es5ify-withFauxDom.

ERROR in dashboard_vendors.js from UglifyJs Unexpected token: operator (>) [./~/react-faux-dom/lib/withFauxDOM.js:4,0][dashboard_vendors.js:87860,38]

With that, I confirm your changes in https://github.com/Olical/react-faux-dom/pull/100 resolve the issue addressed here!

Olical commented 7 years ago

I did npm version patch && npm publish --tag next, so npm install react-faux-dom@4.0.1 should still spot it, but npm install react-faux-dom wouldn't default to it. That's great to hear though! Thank you very much for testing and I apologise for any inconvenience caused.

I should've caught that the ES6 features wouldn't be compiled down for consumers when I merged the changes. I'll pay more attention to that in the future!

Well done everyone, I'll deploy as 4.0.2, let me know if any issues persist after that.

tibotiber commented 7 years ago

Fix is not working! I'm getting this error:

Uncaught TypeError: Cannot read property 'chart' of null
    at Object.render (App.js:26)
    at applySuper (withFauxDOM.js:14)
    at Object.render (withFauxDOM.js:68)
    at ReactCompositeComponent.js:796
    at measureLifeCyclePerf (ReactCompositeComponent.js:75)
    at ReactCompositeComponentWrapper._renderValidatedComponentWithoutOwnerOrContext (ReactCompositeComponent.js:795)
    at ReactCompositeComponentWrapper._renderValidatedComponent (ReactCompositeComponent.js:822)
    at ReactCompositeComponentWrapper.performInitialMount (ReactCompositeComponent.js:362)
    at ReactCompositeComponentWrapper.mountComponent (ReactCompositeComponent.js:258)
    at Object.mountComponent (ReactReconciler.js:46)
    at ReactCompositeComponentWrapper.performInitialMount (ReactCompositeComponent.js:371)
    at ReactCompositeComponentWrapper.mountComponent (ReactCompositeComponent.js:258)
    at Object.mountComponent (ReactReconciler.js:46)
    at mountComponentIntoNode (ReactMount.js:104)
    at ReactReconcileTransaction.perform (Transaction.js:140)
    at batchedMountComponentIntoNode (ReactMount.js:126)
    at ReactDefaultBatchingStrategyTransaction.perform (Transaction.js:140)
    at Object.batchedUpdates (ReactDefaultBatchingStrategy.js:62)
    at Object.batchedUpdates (ReactUpdates.js:97)
    at Object._renderNewRootComponent (ReactMount.js:320)
    at Object._renderSubtreeIntoContainer (ReactMount.js:401)
    at Object.render (ReactMount.js:422)
    at Object.<anonymous> (index.js:6)
    at __webpack_require__ (bootstrap 96c7d4f…:659)
    at fn (bootstrap 96c7d4f…:85)
    at Object.<anonymous> (fetch.js:461)
    at __webpack_require__ (bootstrap 96c7d4f…:659)
    at validateFormat (bootstrap 96c7d4f…:708)
    at bundle.js:712

This is on the test-4.0.1 branch of the minimal example I posted earlier.

tibotiber commented 7 years ago

@Olical I've invited you to that repo. Thanks for the invite here by the way, feeling proud 😊.

Olical commented 7 years ago

Oh no, I'll have to sort this after work now. That's really strange, I must have bound something incorrectly :frowning:

tibotiber commented 7 years ago

I just checked the code. It seems to me that the component returned by the HOC does not extend the one provided as argument, it only reuse some of the lifecycle hooks and render method. So anything else, such as state defined in the constructor, or class properties and methods defined by the user on the passed component, are not present on the returned component.

This is exactly were the implementation of the inheritance inversion was causing me trouble to implement.

@sdhicks can you confirm whether your test code was using the withFauxDom HOC? Or maybe you changed lib but the package.json main was pointing to dist (if you played around with that)?

Olical commented 7 years ago

Oh balls, my bad. I'll try to fix the extension later, there must be a way to do it.

sdhicks commented 7 years ago

@tibotiber a fresh install of "react-faux-dom": "4.0.2" seems to work for me here

tibotiber commented 7 years ago

@sdhicks are you using the withFauxDom HOC?

sdhicks commented 7 years ago

@tibotiber Ah, I am not - that's why I didn't see this error crop up when testing. I am simply using react-faux-dom for rendering a word cloud using d3 and d3-cloud, ie:

this.wordCloud = ReactFauxDom.createElement('div'); and after some cloud layout code return this.wordCloud.toReact();

I just started using this dependency and am very interested in learning other pros in using it! But I do want to apologize in not catching the error when I gave the all clear 😢

tibotiber commented 7 years ago

Ahah, no problem! At least we have consistent behaviour. Thanks for your inputs and ideas here :+1:.

Olical commented 7 years ago

As far as I can tell so far, to do a HOC you need to render the child like this

    render: function (props) {
      return React.createElement(WrappedComponent, props)
    }

And pass all of your methods through as props. Sound about right? I kinda like this since it'll work with stateless function components too.

tibotiber commented 7 years ago

HOCs have varied purpose, props proxying is one of them which I found to be the wrong abstraction for our use-case when I implemented withFauxDom. I'd advise checking this article which explains the different techniques for HOCs shortly but with good depth.

The way the Mixin worked was to manipulate the component instance's state using its instance methods. I find it natural for the behaviour we try to implement, which is why I tried to keep that mindset. There are ways to do this with refs (I believe) but I find them kinda hacky (and you can't do anything until the component is mounted). The inheritance inversion here is an elegant solution in my opinion. I agree that class inheritance should be avoided in general as this is not the best way to compose React components, but I do find it to be a good fit for this particular use-case. Note that this is the first time I'm actually using class inheritance and I am generally advocating OLOO vs OOP in JS, so I'm not a fan of classes normally.

I suddenly thought of https://github.com/Olical/react-faux-dom/pull/91#issuecomment-300242620, which I wrote when implementing the HOC: the idea here is we would actually be happy avoiding the inheritance inversion and asking users to extend a ReactFauxDOM.Component that has specific behaviours (using instance methods to manipulate the component state), exactly like the React team recommends React.PureComponent for the same kind of reasons. The only issue is that this lets users override lifecycle methods that are necessary for the ReactFauxDom.Component to work properly. Hence we would need to highlight in the docs that some methods need to be called with super() if the user overrides them. This is why I chose the inheritance inversion, it puts the class extension on our side and we can take care of calling super() if needed, which is simpler and safer for the user.

I'm not pushing for the inheritance inversion or class inheritance here, I just want to share the implementation choices I have silently made so we can make a better group decision on a potential rewrite 🤓. Let me know if I am not clear, I realise this might be a bit dense and I find it hard to explain concisely 😕.

tibotiber commented 7 years ago

In French we say only crazy people don't change their mind. I had to write down this whole comment explaining my idea to realise it was not that right finally. The props proxying is the right approach. Pushed #102 to implement it :).