Refactor inheritance in babel-plugin-transform-classes #7444

thymikee · 2018-02-27T16:57:38Z

Q	A
Fixed Issues?	Fixes #7202
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR
Any Dependency Changes?
License	MIT

Implementing loose type of classes transform as a class extending from vanilla transform seems a bit indirect, cause confusion and artificial constructs like here. To simplify things I've decided to merge them into one class and expose as a function to not promote further inheritance.

By the way I've added // @flow pragma to the files and adjusted some typings (but not all of them, I'm not sure how to type Babel so help appreciated)

@Andarist let me know if that's what you were thinking about, or maybe I got it wrong.
cc @xtuc

babel-bot · 2018-02-27T17:18:17Z

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/7312/

Andarist · 2018-02-27T18:39:44Z

Im wondering if we could just iterate through given class to collect "meta" object describing the class, smth like:

{ methods, constructor, super, }

(not that i havent analyzed the source to see what properties we would have to gather).

And use that collected meta (mostly consisted of PathNodes) to construct conditionally (based on the meta & loose) the final es5 class. Would that make sense to you? Would it make it more readable in your eyes?

I kinda feel that having a class here (in its current form at least) is kinda clunky and the same output could be achieved by "just functions".

xtuc · 2018-02-28T12:27:21Z

I kinda feel that having a class here (in its current form at least) is kinda clunky and the same output could be achieved by "just functions".

I agree with that.

Looking at the code and your change it feels like we have removed the inheritance in favor of conditions, which is probably inevitable but would be clearer with just functions.

thymikee · 2018-02-28T12:30:07Z

Yep, I'm planning to move this to functions 👍.
Some conditions were already there, so the separation was artificial (and undone). Let's treat it as a in-progress state, I'll try to update this today.

thymikee · 2018-02-28T19:39:56Z

My battery is running low, so I'll just commit some wip stuff. I've moved class to a function with a simple object as a state. I also found pretty flexible to update the state with a function setState() (can be refactored to other data structure, or debugged easily).
Let me know what do you think of this direction.

thymikee · 2018-03-01T22:15:39Z

I think this is ready to review. Some things got simplified throughout the process, but not much. I recommend reviewing with ?w=1 query param :)

thymikee · 2018-03-16T16:52:11Z

FYI, just rebased to master

xtuc · 2018-03-17T18:04:50Z

Thanks, I'll review your work shortly.

Andarist

LGTM, we can always work on it more later if anyone would be interested in doing so

Andarist · 2018-03-19T09:26:57Z

packages/babel-plugin-transform-classes/src/vanilla.js

   */
-
-  constructorMeMaybe() {
+  function createConstructor() {


nitpick - from what I observed maybe* is a convention used in babel's codebase to indicate that a function attempts to do something, but may bail out - imho it might be good to preserve it here as this function won't always "create constructor" 😉

Renamed it to maybeCreateConstructor. See last 2 commits.

thymikee · 2018-03-19T15:39:44Z

Thanks @Andarist & @xtuc! :)

xtuc · 2018-03-19T16:38:07Z

Thanks for your contribution!

loganfsmyth · 2018-03-20T06:38:21Z

packages/babel-plugin-transform-classes/src/transformClass.js

@@ -0,0 +1,718 @@
+// @flow


It looks like this file was given Flow annotations, but fails a few checks. Would someone be up for taking a look? Looks like just a couple failures.

Oh, sorry about that! I wasn't sure how to type it in a usable way (without blindly patching) 😬

No problem. I'd say we should just remove the @flow annotation if it isn't actually intended to typecheck them. The flow type definitions are still useful for documentation, so those are fine though.

* master: (140 commits) Update to beta.42 (babel#7609) Disable flow on transformClass, fix preset-env errors (babel#7605) Logical Assignment: ensure computed key isn't recomputed (babel#7604) Remove obsolete max-len eslint rule and reformat some stuff to fit (babel#7602) Centralize Babel's own compilation config to make it easier to follow. (babel#7599) Run prettier to format all JSON. Tweak es2015-related plugin order in preset-env (babel#7586) Refactored quirky inheritance in babel-plugin-transform-classes (babel#7444) Add RegExp support to include/exclude preset-env options (babel#7242) v7.0.0-beta.42 Use strict namespace behavior for mjs files. (babel#7545) Remove outdated spec deviation note [skip ci] (babel#7571) Ensure that the backward-compat logic for plugin-utils copies over the version API properly. (babel#7580) Rename actual/expected test files to input/output (babel#7578) Use helper-module-import inside entry plugin too Use helper-module-imports instead of custom import (babel#7457) Fix "Module build failed: Error: Cannot find module '@babel/types'" (babel#7575) Wrap wrapNativeSuper helpers in redefining functions for better tree-shakeability (babel#7188) Favour extends helper over objectWithoutProperties when whole object gets copied anyway (babel#7390) Fix incorrect value of _cache in _wrapNativeSuper (babel#7570) ...

xtuc changed the title ~~chore(plugin-transform-classes): refactor inheritance~~ Refactor inheritance in babel-plugin-transform-classes Feb 28, 2018

thymikee force-pushed the chore/refactor-transform-class branch from 420cd6d to 31b494d Compare February 28, 2018 19:42

thymikee added 8 commits March 16, 2018 17:47

chore(plugin-transform-classes): refactor inheritance

0a6a3f6

path some flow types

0bf6fe8

remove unused export

03d46dc

get rid of class

789ec56

temporarily rename to vanilla for diff readability

33a362a

more setState with minor adjustments

a7a3ae9

rename 'private' methods

2c4b847

explicit null check

cfcff06

thymikee force-pushed the chore/refactor-transform-class branch from a58c017 to cfcff06 Compare March 16, 2018 16:49

adjust import ordering

f423ec3

existentialism added the PR: Internal 🏠 A type of pull request used for our changelog categories label Mar 16, 2018

Andarist approved these changes Mar 19, 2018

View reviewed changes

Andarist reviewed Mar 19, 2018

View reviewed changes

thymikee added 2 commits March 19, 2018 11:00

Rename createConstructor -> maybeCreateConstructor

0b1db39

Rename vanilla.js -> transformClass.js

e21fe57

xtuc approved these changes Mar 19, 2018

View reviewed changes

Andarist merged commit e2c5f25 into babel:master Mar 19, 2018

thymikee deleted the chore/refactor-transform-class branch March 19, 2018 15:38

loganfsmyth reviewed Mar 20, 2018

View reviewed changes

thymikee mentioned this pull request Mar 20, 2018

Disable flow on transformClass, fix preset-env errors #7605

Merged

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor inheritance in babel-plugin-transform-classes #7444

Refactor inheritance in babel-plugin-transform-classes #7444

thymikee commented Feb 27, 2018 •

edited

Loading

babel-bot commented Feb 27, 2018 •

edited

Loading

Andarist commented Feb 27, 2018

xtuc commented Feb 28, 2018

thymikee commented Feb 28, 2018

thymikee commented Feb 28, 2018

thymikee commented Mar 1, 2018

thymikee commented Mar 16, 2018

xtuc commented Mar 17, 2018

Andarist left a comment

Andarist Mar 19, 2018

thymikee Mar 19, 2018

thymikee commented Mar 19, 2018

xtuc commented Mar 19, 2018

loganfsmyth Mar 20, 2018

thymikee Mar 20, 2018

loganfsmyth Mar 20, 2018

Refactor inheritance in babel-plugin-transform-classes #7444

Refactor inheritance in babel-plugin-transform-classes #7444

Conversation

thymikee commented Feb 27, 2018 • edited Loading

babel-bot commented Feb 27, 2018 • edited Loading

Andarist commented Feb 27, 2018

xtuc commented Feb 28, 2018

thymikee commented Feb 28, 2018

thymikee commented Feb 28, 2018

thymikee commented Mar 1, 2018

thymikee commented Mar 16, 2018

xtuc commented Mar 17, 2018

Andarist left a comment

Choose a reason for hiding this comment

Andarist Mar 19, 2018

Choose a reason for hiding this comment

thymikee Mar 19, 2018

Choose a reason for hiding this comment

thymikee commented Mar 19, 2018

xtuc commented Mar 19, 2018

loganfsmyth Mar 20, 2018

Choose a reason for hiding this comment

thymikee Mar 20, 2018

Choose a reason for hiding this comment

loganfsmyth Mar 20, 2018

Choose a reason for hiding this comment

thymikee commented Feb 27, 2018 •

edited

Loading

babel-bot commented Feb 27, 2018 •

edited

Loading