Skip to content

Conversation

akorchev
Copy link
Collaborator

@akorchev akorchev commented Feb 7, 2016

Should fix #41

@akorchev akorchev changed the title Attempt to support native ES2016 classes Attempt to support native ES2015 classes Feb 7, 2016
.babelrc Outdated
"env": {
"development": {
"plugins": ["transform-decorators-legacy"],
"presets": ["es2015", "stage-0", "react"]
Copy link
Owner

Choose a reason for hiding this comment

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

Rather than use presets here let's just list plugins explicitly in every configuration? We can then move the shrared plugins up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do. Should I modify package.json as well to include only the required plugins or keep the presets?

@akorchev
Copy link
Collaborator Author

akorchev commented Feb 8, 2016

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

@akorchev
Copy link
Collaborator Author

akorchev commented Feb 8, 2016

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
Copy link
Owner

gaearon commented Feb 8, 2016

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

@akorchev
Copy link
Collaborator Author

akorchev commented Feb 8, 2016

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

@gaearon
Copy link
Owner

gaearon commented Feb 8, 2016

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
Copy link
Collaborator Author

akorchev commented Feb 8, 2016

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
Copy link
Owner

gaearon commented Feb 8, 2016

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
Copy link
Collaborator Author

akorchev commented Feb 8, 2016

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
Copy link
Owner

gaearon commented Feb 8, 2016

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

@akorchev
Copy link
Collaborator Author

akorchev commented Feb 9, 2016

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

@jlongster
Copy link

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

@akorchev
Copy link
Collaborator Author

akorchev commented Mar 4, 2016

This pull request should do the trick. Tests pass.

gaearon added a commit that referenced this pull request Mar 4, 2016
Attempt to support native ES2015 classes
@gaearon gaearon merged commit 7836d4a into gaearon:master Mar 4, 2016
@jlongster
Copy link

Thanks @akorchev and @gaearon !

@gaearon
Copy link
Owner

gaearon commented Mar 4, 2016

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

@akorchev
Copy link
Collaborator Author

akorchev commented Mar 4, 2016

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

@gaearon
Copy link
Owner

gaearon commented Mar 4, 2016

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
Copy link
Collaborator Author

akorchev commented Mar 4, 2016

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
Copy link
Collaborator Author

akorchev commented Mar 4, 2016

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

@gaearon
Copy link
Owner

gaearon commented Mar 4, 2016

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
Copy link
Owner

gaearon commented Mar 4, 2016

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

@gaearon
Copy link
Owner

gaearon commented Mar 4, 2016

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

@akorchev
Copy link
Collaborator Author

akorchev commented Mar 4, 2016

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
Copy link
Collaborator Author

akorchev commented Mar 4, 2016

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

@akorchev
Copy link
Collaborator Author

akorchev commented Mar 4, 2016

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
Copy link
Owner

gaearon commented Mar 4, 2016

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

@gaearon
Copy link
Owner

gaearon commented Mar 4, 2016

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

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.

3 participants