gaearon / react-proxy

Proxies React components without unmounting or losing their state
460 stars 50 forks source link

Attempt to support native ES2015 classes #42

Closed akorchev closed 8 years ago

akorchev commented 8 years ago

Should fix https://github.com/gaearon/react-proxy/issues/41

akorchev commented 8 years ago

Strangely the Travis build fails with TypeError: Cannot redefine property: name. Tests pass on my side though. Node versions?

akorchev commented 8 years ago

It seems to be the node version which the Travis build uses. From MDN:

Notice that in non-standard, pre-ES6 implementations the configurable attribute was false as well.

@gaearon should I revert the latest commit then?

gaearon commented 8 years ago

Is it configurable in modern browsers? Those are the ones we care most about.

akorchev commented 8 years ago

Yes in Chrome, no in Safari. I guess revert then?

gaearon commented 8 years ago

Yeah :-( Will have to look for other way. At this point the only problem is that we need to find some other way of testing the dynamic code path.

akorchev commented 8 years ago

If I keep the switch statement the test passes. This may be OK as a react component can only have at most three constructor parameters AFAIK - props, context and updater.

gaearon commented 8 years ago

I understand but it is still weird that we keep this code to appease a test. Better to find a way to fix the test.

akorchev commented 8 years ago

I tried to debug it but I can't get the source maps to work. Can't find a way to fix the test either.

gaearon commented 8 years ago

Can we maybe define fake global.Function but make sure it has a valid prototype? Could this be the issue with accessing call?

akorchev commented 8 years ago

Inheriting from Function and throwing in the constructor seems to do the trick.

jlongster commented 8 years ago

I just hit this as well. Is anyone still actively working on this?

akorchev commented 8 years ago

This pull request should do the trick. Tests pass.

jlongster commented 8 years ago

Thanks @akorchev and @gaearon !

gaearon commented 8 years ago

I’ll need to backport this to 1.x because that’s what everybody uses right now, hopefully that will be straightforward :-)

akorchev commented 8 years ago

@gaearon code looks quite similar. I can do the backporting. Thoughts?

gaearon commented 8 years ago

I pushed some work in progress in 1.x branch but it’s broken right now. Would appreciate fixes.

Also, I can’t get Webpack build to work (npm run build). We want the compiled code to not rely on arrow functions or classes. Would a bundle compiled in babel-classes env still be usable for native-classes? Can you look into fixing the build as well?

akorchev commented 8 years ago

I just ran npm run build in my fork successfully. Do I need to do something else? What is the error that you are getting?

akorchev commented 8 years ago

Nvm. I guess you meant things broke after merging my pull request. Will look into it.

gaearon commented 8 years ago

Apologies, seems like I broke it. I didn’t like that we referenced harmony as opt-in inside package.json but .babelrc was reversed and used “development” (which is not really representative of what the option meant).

gaearon commented 8 years ago

I reverted my commit but I’d appreciate if you could rework it in a way that would be correct later.

gaearon commented 8 years ago

1.x almost works, has one test failing. Mind looking into it?

akorchev commented 8 years ago

I used 'development' because this is the default 'environment' babel runs with. I decided to make the 'harmony' the odd one to avoid specifying the node environment in the npm scripts. We should continue building with babel classes though in order to support older runtimes. I will check the 1.x branch.

akorchev commented 8 years ago

The other solution I can think of is to create two babel environments and duplicate most of the plugins.

akorchev commented 8 years ago

Fixed 1.x. Let me know if you want me to make any changes in the .babelrc. Right now I see the following options:

  1. Keep it as it is. Will work as long as the default babel environment remains 'development'.
  2. Make a specific environment for babel classes. Specify it in the build script and everywhere else it is needed.
gaearon commented 8 years ago

I’d vote for a specific env + duplicating plugins explicitly.

gaearon commented 8 years ago

Out in 1.1.3 (2.0.2 too although nobody uses 2.x yet probably).