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

Remove ClassCallCheck, possibleConstructorReturn in loose mode #4850

Merged
merged 4 commits into from Jun 26, 2017

Conversation

Projects
None yet
8 participants
@hzoo
Member

hzoo commented Nov 16, 2016

Q A
Bug fix?
Breaking change?
New feature? yes
Deprecations?
Spec compliancy?
Tests added + pass? yes
Fixed tickets
License MIT
Doc PR
Dependency Changes

cc @developit

@codecov-io

This comment has been minimized.

codecov-io commented Nov 16, 2016

Codecov Report

Merging #4850 into 7.0 will increase coverage by <.01%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##              7.0    #4850      +/-   ##
==========================================
+ Coverage   85.21%   85.21%   +<.01%     
==========================================
  Files         284      284              
  Lines        9958     9966       +8     
  Branches     2780     2785       +5     
==========================================
+ Hits         8486     8493       +7     
  Misses        971      971              
- Partials      501      502       +1
Impacted Files Coverage Δ
packages/babel-helpers/src/helpers.js 100% <100%> (ø) ⬆️
...bel-plugin-transform-es2015-classes/src/vanilla.js 90.9% <90.9%> (+0.27%) ⬆️
packages/babel-traverse/src/visitors.js 85.71% <0%> (-0.96%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cce83a0...8a4ab08. Read the comment docs.

@loganfsmyth

This comment has been minimized.

Member

loganfsmyth commented Nov 16, 2016

Wasn't it possibleConstructorReturn that @developit was asking about?

@developit

This comment has been minimized.

developit commented Nov 16, 2016

Both actually. I'm looking for a way to say "hey, don't worry about protecting classes, they are internal to my module anyway".

@DrewML

This comment has been minimized.

Member

DrewML commented Nov 17, 2016

Not that I have anything against this change, but I'm just curious to know what the use-case is @developit. Performance?

@developit

This comment has been minimized.

developit commented Nov 17, 2016

Performance and file size. I use codemods or avoid classes entirely, with this in place it would be more viable.

@hzoo

This comment has been minimized.

Member

hzoo commented Nov 17, 2016

@developit oh and you can also check out babili's plugins + babel codemods

@DrewML

This comment has been minimized.

Member

DrewML commented Nov 17, 2016

Part of me wonders if these things should just be part of loose mode for es2015-classes, rather than separate options. Thoughts @hzoo?

@developit

This comment has been minimized.

developit commented Nov 17, 2016

@hzoo think babili would cover stripping these? That would get me to upgrade to 6 for sure! 😂

@developit

This comment has been minimized.

developit commented Nov 17, 2016

@DrewML huge fringe benefit of doing that would be that the es2015 preset would be able to enable these mods with a single option. Currently disassembling that preset is a bit of a hard battle see modify-babel-preset haha).

Perhaps another option is a "minimal" mode? Or maybe that's encroaching on Babili territory.

@DrewML

This comment has been minimized.

Member

DrewML commented Nov 17, 2016

I think minimal mode was the initial intent of loose (but who knows at this point).

Babel 5 docs on loose mode were:

Loose mode enables certain transformers to generate cleaner output that lacks specific ES6 edgecases. These edgecases are either unlikely to appear in your code or the inclusion of them introduces significant overhead.

As a result of loose mode code will execute faster and be much more readable and comparable to the original but will deviate from the spec in slight ways.

I kind of interpret that as "generate the minimal amount of changes to make the code work, and ignore edge-cases where someone could do something bad"

@developit

This comment has been minimized.

developit commented Nov 17, 2016

That is certainly how I think of it. It would be a very compelling option to have handy, at least for me. Tends to be like 300-500 bytes, which for small libs like preact is a heck of a lot (10%!).

@DrewML

This comment has been minimized.

Member

DrewML commented Nov 17, 2016

@hzoo Thoughts/Opinions on combining this into loose mode?

@hzoo

This comment has been minimized.

Member

hzoo commented Nov 17, 2016

We could? I guess it would be a "breaking change" still? although most that use loose mode wouldn't care about it anyway

@developit

This comment has been minimized.

developit commented Nov 17, 2016

It be breaking for anything that is relying on calling constructors to throw (for whatever reason, I can't think of a good one, maybe some over-testing?). It's just the preset though and I would guess as you said, those are not the people running in loose mode. Also since loose was only added in 6.13 (?) it seems like it'd be not quite as widely used yet.

@hzoo

This comment has been minimized.

Member

hzoo commented Nov 17, 2016

This changes the plugin not the preset so it would affect everyone using the option which should of been there since the beginning? Yeah it's removing a throw check so it wouldn't break anything in the opposite way

@DrewML

This comment has been minimized.

Member

DrewML commented Nov 17, 2016

I guess this kind of leads to the bigger question of "what is a breaking change" in these options. In theory, we could never add any features to loose mode, because any additional optimization could always be a breaking change.

Semver is hard :(

@hzoo hzoo referenced this pull request Apr 6, 2017

Closed

optional _classCallCheck? #798

@Download

This comment has been minimized.

Download commented Apr 7, 2017

Using loose mode is the way to go for both classCallCheck and possibleConstructorReturn if you ask me.

I would like for Babel to generate code that is as close to ES6 compliant as possible under normal circumstances, then use loose mode to drop every line of code that is not strictly necessary to make stuff just work. It means in loose mode you can do stuff that is against spec... But probably you will dev in strict mode and catch such uses there. Once it's in production, use loose mode for max. performance.

Having a single toggle on the preset would be infinitely more valuable than having to control each plugin separately.

@developit

This comment has been minimized.

developit commented Apr 7, 2017

Also makes fall-through from presets like latest easier since it's just the existing loose option, right?

@hzoo

This comment has been minimized.

Member

hzoo commented Apr 7, 2017

Yes it should although fyi latest is deprecated since env covers all it's functionality

@developit

This comment has been minimized.

developit commented Apr 7, 2017

You know I'm behind on all that stuff (still using 5 in places haha)

@hzoo

This comment has been minimized.

Member

hzoo commented Apr 7, 2017

😅 can someone just make the auto upgrade from 5 to 6 bot

@developit

This comment has been minimized.

developit commented Apr 7, 2017

@hzoo still produces larger output for some reason, need to look into why. I'm basically only using babel to convert let and const to var without caring about TDZ or block scoping.

@hzoo hzoo changed the title from removeClassCallCheck option to Remove ClassCallCheck in loose mode Jun 9, 2017

@hzoo hzoo changed the base branch from master to 7.0 Jun 9, 2017

@hzoo hzoo changed the title from Remove ClassCallCheck in loose mode to Remove ClassCallCheck, possibleConstructorReturn in loose mode Jun 9, 2017

@@ -0,0 +1,40 @@
function _inherits(subClass, superClass) { if (typeof superClass !== "function" && superClass !== null) { throw new TypeError("Super expression must either be null or a function, not " + typeof superClass); } subClass.prototype = Object.create(superClass && superClass.prototype, { constructor: { value: subClass, enumerable: false, writable: true, configurable: true } }); if (superClass) Object.setPrototypeOf ? Object.setPrototypeOf(subClass, superClass) : subClass.__proto__ = superClass; }

This comment has been minimized.

@hzoo

hzoo Jun 9, 2017

Member

Do we want to remove the checks here too?

function _inheritsLoose(subclass, superClass) {
  subClass.prototype = Object.create(superClass && superClass.prototype);
  subClass.prototype.constructor = subClass;
  if (superClass) {
    subClass.__proto__ = superClass;
  } 
}

This comment has been minimized.

@developit

developit Jun 15, 2017

Definitely. My view here is that people using loose mode classes are just asking for a direct replacement for the ES5 equivalent.

function C() {
woops.super.test();
var _this = _B.call(this) || this;

This comment has been minimized.

@hzoo

hzoo Jun 9, 2017

Member

can drop || this as well

@Download

This comment has been minimized.

Download commented Jun 15, 2017

@hzoo Loving what you are doing here! Can't wait for it to be merged and released!

function A(track) {
if (track !== undefined) {
var _this = _B.call(this, track) || this;

This comment has been minimized.

@hzoo

hzoo Jun 19, 2017

Member

and remove || this I guess?

(function (subClass, superClass) {
subClass.prototype = Object.create(superClass && superClass.prototype);
subClass.prototype.constructor = subClass;
if (superClass) {

This comment has been minimized.

@developit

developit Jun 19, 2017

Thoughts on also removing this check?

This comment has been minimized.

@hzoo

hzoo Jun 23, 2017

Member

because we would assume superClass is not null? like class extends null or class extends expression

This comment has been minimized.

@developit

developit Jun 23, 2017

good call, I was thinking it would always be appropriate to throw an exception but I stand corrected! :)

This comment has been minimized.

@hzoo

hzoo Jun 24, 2017

Member

Ok we can just assume in loose mode that the superClass is non-null and has a prototype to get

  (function (subClass, superClass) {
    subClass.prototype = Object.create(superClass.prototype);
    subClass.prototype.constructor = subClass;
    subClass.__proto__ = superClass;
  })

@hzoo hzoo requested review from loganfsmyth and jridgewell Jun 19, 2017

@@ -377,6 +377,16 @@ helpers.inherits = template(`
})
`);
helpers.inheritsLoose = template(`
(function (subClass, superClass) {
subClass.prototype = Object.create(superClass && superClass.prototype);

This comment has been minimized.

@hzoo

hzoo Jun 23, 2017

Member

I guess we would be able to remove the check here too then @developit? superClass &&

This comment has been minimized.

@developit
woops.super.test();
var _this = babelHelpers.possibleConstructorReturn(this, _Foo.call(this));
var _this = _Foo.call(this) || this;

This comment has been minimized.

@hzoo

hzoo Jun 23, 2017

Member

should we remove || this?

This comment has been minimized.

@jridgewell

jridgewell Jun 23, 2017

Member

If we do, we'd have to remove the whole var _this = (and all the _thiss) too. I think this is fine for now.

This comment has been minimized.

@developit

developit Jun 27, 2017

hmm. this seems important.

woops.super.test();
var _this = babelHelpers.possibleConstructorReturn(this, _Foo.call(this));
var _this = _Foo.call(this) || this;

This comment has been minimized.

@jridgewell

jridgewell Jun 23, 2017

Member

If we do, we'd have to remove the whole var _this = (and all the _thiss) too. I think this is fine for now.

@xtuc

xtuc approved these changes Jun 26, 2017

@hzoo

This comment has been minimized.

Member

hzoo commented Jun 26, 2017

Ok will merge and someone can do #4850 (comment) as a followup issue/PR

@hzoo

This comment has been minimized.

Member

hzoo commented Jun 26, 2017

Also something about travis is messed up (circle ci passes + local)

@hzoo hzoo merged commit bb5528b into 7.0 Jun 26, 2017

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
ci/circleci Your tests passed on CircleCI!
Details

@xtuc xtuc deleted the removeClassCallCheck-option branch Jun 27, 2017

@Download

This comment has been minimized.

Download commented Jul 1, 2017

@hzoo Is there a way to test this in my project before it is released to NPM?

@hzoo

This comment has been minimized.

Member

hzoo commented Jul 1, 2017

I could some better docs for this (or a utility to make it easy) but yeah you can just overwrite your node_modules? Like literally replace the 2 files that we changed. You can probably just run the src/index.js through babeljs.io/repl and then replace the lib/index.js in node_modules and add that helper to test it

@Download

This comment has been minimized.

Download commented Jul 3, 2017

@hzoo Ok. I guess that's a bit too far outside of my comfort zone :) I guess I'll wait until the next alpha makes it to NPM. :)

@hzoo

This comment has been minimized.

Member

hzoo commented Jul 12, 2017

@hzoo hzoo referenced this pull request Sep 16, 2017

Merged

Babel 7 #23966

2 of 2 tasks complete

yiminghe added a commit to yiminghe/babel-runtime-loose that referenced this pull request Oct 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment