Shopify / javascript

The home for all things JavaScript at Shopify.
MIT License
253 stars 38 forks source link

Verify shared instance properties -> class properties #84

Closed lemonmade closed 8 years ago

lemonmade commented 8 years ago

Noted in the second example of this issue: https://github.com/juliankrispel/decaf/issues/127.

Currently, we are translating this:

class Foo
  bar: 123

to this:

class Foo {
  bar = 123;
}

But these have different semantics: the CoffeeScript one goes on the prototype (shared between all instances) where the JavaScript one goes on the instance.

GoodForOneFare commented 8 years ago

@lemonmade this is another one that'll now throw an error during decaf. FWIW, there's 80+ affected files, so I think there's a strong case for automating a fix.

lemonmade commented 8 years ago

What do you think the automated fix should look like? Should we add a Foo.prototype.bar = 123 to decaf?

GoodForOneFare commented 8 years ago

Assuming that we split all classes into separate files (#125), I think we can insert any private stuff before the class definition. So:

class Shopify.AutocompleteV2
  LIST_CLASS = '.js-autocomplete-suggestions'
  PIXELS_FROM_BOTTOM_TO_INFINITE_SCROLL = 500
  ERROR_DISPLAY_TIME = 5000

  constructor: (node, @renderer, @options={}) ->

becomes

const LIST_CLASS = '.js-autocomplete-suggestions'
const PIXELS_FROM_BOTTOM_TO_INFINITE_SCROLL = 500
const ERROR_DISPLAY_TIME = 5000

class Shopify.AutocompleteV2
  constructor: (node, @renderer, @options={}) ->
GoodForOneFare commented 8 years ago

Heh. Yeah, prettymuch what you just raised in #157 :)

lemonmade commented 8 years ago

👍

lemonmade commented 8 years ago

Welp, I think the different issues are getting mixed up :P I think we are on the same page and https://github.com/Shopify/javascript/issues/138 partially covers what we are after in terms of shared literal properties.

GoodForOneFare commented 8 years ago

The mainline merge picked up a fix from Julian for this (https://github.com/shopify/decaf/blame/master/test/test.js#L1209). Closing.