kaliberjs / build

Zero configuration, modern, opinionated build tools
https://kaliberjs.github.io/build/
MIT License
10 stars 3 forks source link

Support ECMAScript proposals stage 2 & 3 #9

Closed klaasman closed 7 years ago

klaasman commented 7 years ago

Particularly for features like decorators, object rest/spread & public class fields

// public class fields
class X {
  static y = 'z'

  foo = 'bar'

  baz () {
    return this.foo // 'bar'
  }
}

X.y // 'z'
EECOLOR commented 7 years ago

If we include features from those earlier stages I would rather pick a select few that we think are worth the risk. Have you read this article: JavaScript Developers: Watch Your Language!?

I think object rest / spread would be safe to add. For decorators I am not completely sure.

When you want to wrap a 'functional' component you can simply call the function. So it seems the only appeal is for classes. The problem with classes is that they are not hoisted to the top:

// this works
export default myDecorator(MyFunctionalComponent)

function MyFunctionalComponent() { ... }
// this does not
export default myDecorator(MyClassComponent)

class MyClassComponent { ... }

The weird things is that class MyClassComponent is equivalent to:

function MyClassComponent() {}
MyClassComponent.prototype = { ... }

And in this last case, using a decorator without special syntax would work...

As for static, I do not see much benefit in them, so convince me with a use case :smiley:.

Maybe we should open an issue for each separate feature that you would like to include and add a compelling usecase.

klaasman commented 7 years ago

The static keyword is useful for defining propTypes on a react Component, this is however my only use-case.

EECOLOR commented 7 years ago

These are the stages:

Stage 1: The committee expects to devote time to examining the problem space, solutions and cross-cutting concerns Stage 2: The committee expects the feature to be developed and eventually included in the standard Stage 3: The solution is complete and no further work is possible without implementation experience, significant usage and external feedback. Stage 4: The addition will be included in the soonest practical standard revision

I think stage 3+ features (rest/spread) can be added quite safely. Both static and @decorator are stage 2. In case they don't make it to the final version, or are changed underway we need to make sure that fixing the code is doable.

For ... it's a bit tricky, but there is a big chance it will be included. We seem to only use the spread part which has an easy alternative: Object.assign()

For static, looking it up is easy, replacing static x with ClassX.x can be done automagically with some effort. Class properties which we seem to use in case of the default state: state = { ... } is harder to replace. The benefit however is that we do not need to define a constructor.

For @decorator, looking it up is easy, replacing @decorator with decorator(ClassX) can be done automagically with some effort.

EECOLOR commented 7 years ago

A few arguments in favor of adding these features:

rest/spread The spread part removes the noise of Object.assign({}, ...) and advocates immutability which improves understandability and lowers the chance of programming errors. React also includes the spread operator: JSX In Depth - spread attributes

I have never needed the rest part for objects so I can not find any compelling arguments apart from it being consistent with rest/spread of array.

class properties static allows us to define these static properties at the top of the class. This makes components more readable in my opinion, especially when used for PropTypes. It is annoying that classes are not hoisted to the top, if they were this feature would not be needed.

Class instance properties help in reducing this noise. On top of that, they allow us to prevent the noise of a constructor which we rarely need (and never in react components).

decorators We seem to only use class decorators. They allow us to write our code in a more declerative style which lowers the chance of errors and increases reausability. Since they apply to the whole class it greatly improves readability and understandability of a class if they can be placed at the top of the file. This again is only needed because classes are not hoisted to the top.

EECOLOR commented 7 years ago

Since two of these features are stage 2 and only exist to compensate for lack of class hoisting here a few examples to visualize the difference with and without these features.

With features:

@withData({ location: "..." })
export default class Test extends Component {
  static propTypes = {
    data: string.isRequired
  }

  state = { a: "..." }

  render() {
   return (<div />)
  }
} 

Without features (few instances):

export default withData({ location: "..." })(Test)

Test.propTypes = {
  data: string.isRequired
}

function Test(...args) {
  Component.apply(this, args)

  this.state = { a: "..." }

  this.render = () => (<div />)
}

Without features (more instances):

export default withData({ location: "..." })(Test)

Test.propTypes = {
  data: string.isRequired
}

function Test(...args) {
  Component.apply(this, args)

  this.state = { a: "..." }
}
Test.prototype = Object.assign(Object.create(Component.prototype), {
  render() {
    return (<div />)
  }
})

The version without features that does not have many instances does not look bad. I wonder if we should choose that version instead of adding the stage 2 features. We rarely (if ever) need the prototype optimization in our real world applications.

Adopting this style of class definition has another benefit: it greatly reduces the requirement of using this. Functions can be scope inside of the constructor and called without the this prefix. Callbacks can be passed around without binding them.

klaasman commented 7 years ago

I'd say we definitely go for the rest/spread and class properties, they're also in use by create-react-app.

About decorators: being able to export classes at the top of the file is great pattern, however I'd rather go for the more universally accepted decorator-syntax, in comparison to your versions "without features".

EECOLOR commented 7 years ago

I totally agree. I'll add the 3 features.