refactor(tracked): minimally invasive stage 2 decorators support #22

buschtoens · 2018-12-11T17:26:39Z

Closes #16.
Supersedes #17.
Relates to #21.

This adds support for stage 2 decorators and the latest @ember-decorators/babel-transforms while changing as few things as possible. It also upgrades to the latest ember-cli-typescript@beta.

In theory, this should be compatible with ember-cli-typescript@1, since this PR uses @ember-decorators/utils, but I would like to add tests for that before merging.

/cc @rwjblue

addon/tracked.ts

buschtoens · 2018-12-11T17:30:26Z

addon/tracked.ts

@@ -90,15 +90,13 @@ function installTrackedProperty(_target: object, key: string | symbol, descripto
  }

  return {
-    configurable: true,
-    // TODO: correcting a misspelling caused chrome to error
-    // writable: true,


FWIW this is intentional. I fixed the same problem in this PR with a detailed analysis: salsify/ember-css-modules#118

buschtoens · 2018-12-11T17:33:13Z

btw, this is actually semantically incorrect:

sparkles-component/addon/tracked.ts

Lines 76 to 86 in e320c6f

    
           if (typeof initializer === 'function') { 
        
             get = function(this: object) { 
        
               if (values.has(this)) { 
        
                 return values.get(this); 
        
               } else { 
        
                 let value = initializer.call(this); 
        
                 values.set(this, value); 
        
                 return value; 
        
               } 
        
             }; 
        
           } else {

Initializers are evaluated eagerly. This initializer is only first run, when the field is accessed.

buschtoens · 2018-12-11T17:35:47Z

addon/tracked.ts

+type TrackedDecorator = TSDecorator & ((...args: string[]) => TSDecorator);
+
+export const tracked: TrackedDecorator = decoratorWithParams((desc, params = []) => {
+  assert(`@tracked - Can only be used on class fields.`, desc.kind === 'field' || desc.kind === 'method');


If you wanna be really correct.

Suggested change

assert(`@tracked - Can only be used on class fields.`, desc.kind === 'field' || desc.kind === 'method');

assert(

`@tracked - Can only be used on class fields.`,

desc.kind === 'field' || desc.kind === 'method' && (

typeof desc.descriptor.get === 'function' ||

typeof desc.descriptor.set === 'function'

)

);

rwjblue

In order to confirm things still work properly, I'd like to get ember-try scenarios for ember-cli-typescript@1 as well.

package.json

addon/tracked.ts

buschtoens · 2018-12-11T23:20:16Z

@rwjblue I was already working on these scenarios and even found some bugs (upstream 😜). Didn't yet get to push it here though. ☺️

addon/tracked.ts

package.json

types/@ember-decorators/utils/decorator.d.ts

buschtoens · 2018-12-18T09:53:01Z

This PR is pending approval and release of ember-decorators/ember-decorators#336, so I can replace 02546ae with the proper release of @ember-decorators/utils.

But other than that, I am done. Would love another round of 👀 on this!

buschtoens · 2018-12-19T16:22:17Z

@rwjblue ember-decorators/ember-decorators#336 is merged. If you can find time to release it, I can update this PR and you can merge and release it as well. 🚀

NullVoxPopuli · 2019-01-02T14:18:32Z

@buschtoens there is a conflict o.o

NullVoxPopuli · 2019-01-02T14:20:34Z

but also, I can confirm this branch is 💯

buschtoens · 2019-01-02T14:49:41Z

Once there's a new release of ember-decorators, I can update this PR and remove the merge conflict in package.json.

Co-Authored-By: buschtoens <buschtoens@gmail.com>

buschtoens · 2019-01-07T10:29:22Z

@rwjblue I've updated to @ember-decorators v4.0.0 and rebased onto the latest master.

ember-cli/ember-compatibility-helpers#31

buschtoens · 2019-01-10T10:24:31Z

While trying to pre-publish this as @buschtoens/sparkles-component@1.3.0-beta.1, I noticed that #26 broke the build (specifically the prepublishOnly step), because tsc complains about the following type errors:

addon/index.ts:3:21 - error TS7016: Could not find a declaration file for module 'ember-compatibility-helpers'. '/Users/janbuschtoens/open-source/sparkles-component/node_modules/ember-compatibility-helpers/index.js' implicitly has an 'any' type.
  Try `npm install @types/ember-compatibility-helpers` if it exists or add a new declaration (.d.ts) file containing `declare module 'ember-compatibility-helpers';`

3 import { gte } from 'ember-compatibility-helpers';
                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


addon/index.ts:17:23 - error TS2345: Argument of type '(owner: ApplicationInstance) => SparklesComponentManager' is not assignable to parameter of type 'string'.

17   setComponentManager((owner: ApplicationInstance) => {

I've submitted ember-cli/ember-compatibility-helpers#31 in an attempt to generate the types upstream. In the meantime, I've added declaration file for it here.

For the setComponentManager API I've updated the existing declaration. Soon we should be able to just upgrade to an Ember version that ships the correct types and be good to go.

rwjblue · 2019-01-10T15:38:10Z

I don't understand, why did #26 pass CI if it broke things?

types/ember-compatibility-helpers.d.ts

buschtoens · 2019-01-10T15:45:17Z

I don't understand, why did #26 pass CI if it broke things?

It passed CI, because we do not lint types in CI. If you check the logs, you can see that ember-cli-typescript does in fact complain about it, but does not fail the build: https://travis-ci.com/rwjblue/sparkles-component/jobs/167248765#L559-L572

However, prepublishOnly / ember ts:precompile does fail. If you would try to cut a release from master now, it would not let you.

rwjblue · 2019-01-10T15:48:50Z

It passed CI, because we do not lint types in CI.

Gotcha! We should add this to prevent exactly this kind of regression...

rwjblue · 2019-01-10T21:15:11Z

Released in 1.3.0

buschtoens commented Dec 11, 2018

View reviewed changes

addon/tracked.ts Outdated Show resolved Hide resolved

buschtoens commented Dec 11, 2018

View reviewed changes

This was referenced Dec 11, 2018

WIP: Stage2 decorators #17

Closed

fix(utils/decorator): handle desc return value ember-decorators/ember-decorators#336

Merged

rwjblue reviewed Dec 11, 2018

View reviewed changes

package.json Outdated Show resolved Hide resolved

addon/tracked.ts Outdated Show resolved Hide resolved

mike-north reviewed Dec 12, 2018

View reviewed changes

rwjblue mentioned this pull request Dec 26, 2018

going decorators stage-2 #25

Closed

buschtoens and others added 15 commits January 7, 2019 11:25

chore(deps): upgrade decorator related dependencies

bce9b0f

refactor(tracked): minimally invasive stage 2 decorators support

3fd3ea9

chore(deps): use tilde range for @types/ember

fce9928

Co-Authored-By: buschtoens <buschtoens@gmail.com>

chore(deps): use tilde range for @types/ember-qunit

009d874

Co-Authored-By: buschtoens <buschtoens@gmail.com>

chore(deps): use tilde range for @types/ember__test-helpers

6bea49f

Co-Authored-By: buschtoens <buschtoens@gmail.com>

chore(deps): use tilde range for @types/ember-test-helpers

b559e7e

Co-Authored-By: buschtoens <buschtoens@gmail.com>

test: run tests for ember-cli-typescript@1

80e9735

fix(tracked): handle missing desc for unitialized fields in e-c-ts@1

666de51

chore(deps): update yarn.lock

e345174

chore(deps): remove ember-cli-typescript-blueprints

e47eb71

chore(tracked): return void

51bee13

chore(deps): upgrade @types/ember

12d26a2

refactor(tracked): fix type errors with addObserver

ec55d1c

chore(deps): upgrade ember-cli-typescript and ember-cli-babel

c5a0dda

chore(deps): downgrade typescript@^2.9.2

4ea3b97

buschtoens added 2 commits January 7, 2019 11:25

chore(deps): temporarily use pre-release of @e-d/utils

6a614f3

chore(deps): upgrade to @e-d v4.0.0

d0f93f6

buschtoens force-pushed the refactor/stage-2-decorators branch from d22743c to d0f93f6 Compare January 7, 2019 10:26

buschtoens added 2 commits January 10, 2019 11:07

chore(types): add ember-compatibility-helpers

e72fcfe

ember-cli/ember-compatibility-helpers#31

chore(types): add types for new setComponentManager

158873b

rwjblue reviewed Jan 10, 2019

View reviewed changes

types/ember-compatibility-helpers.d.ts Outdated Show resolved Hide resolved

chore(types): remove unnecessary types

048c4fc

rwjblue approved these changes Jan 10, 2019

View reviewed changes

rwjblue merged commit e3e51e8 into rwjblue:master Jan 10, 2019

buschtoens mentioned this pull request Jan 11, 2019

[WIP] feat(tracked): auto tracking #21

Closed

rtablada mentioned this pull request Jun 27, 2019

Ember 3.10 and Stage 1 Decorators (the redecorating) #31

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(tracked): minimally invasive stage 2 decorators support #22

refactor(tracked): minimally invasive stage 2 decorators support #22

buschtoens commented Dec 11, 2018

buschtoens Dec 11, 2018

buschtoens commented Dec 11, 2018

buschtoens Dec 11, 2018 •

edited

rwjblue left a comment

buschtoens commented Dec 11, 2018

buschtoens commented Dec 18, 2018

buschtoens commented Dec 19, 2018

NullVoxPopuli commented Jan 2, 2019

NullVoxPopuli commented Jan 2, 2019

buschtoens commented Jan 2, 2019

buschtoens commented Jan 7, 2019

buschtoens commented Jan 10, 2019

rwjblue commented Jan 10, 2019

buschtoens commented Jan 10, 2019

rwjblue commented Jan 10, 2019

rwjblue commented Jan 10, 2019

-  assert(`@tracked - Can only be used on class fields.`, desc.kind === 'field' || desc.kind === 'method');
+  assert(
+    `@tracked - Can only be used on class fields.`,
+    desc.kind === 'field' || desc.kind === 'method' && (
+      typeof desc.descriptor.get === 'function' ||
+      typeof desc.descriptor.set === 'function'
+    )
+  );

refactor(tracked): minimally invasive stage 2 decorators support #22

refactor(tracked): minimally invasive stage 2 decorators support #22

Conversation

buschtoens commented Dec 11, 2018

buschtoens Dec 11, 2018

Choose a reason for hiding this comment

buschtoens commented Dec 11, 2018

buschtoens Dec 11, 2018 • edited

Choose a reason for hiding this comment

rwjblue left a comment

Choose a reason for hiding this comment

buschtoens commented Dec 11, 2018

buschtoens commented Dec 18, 2018

buschtoens commented Dec 19, 2018

NullVoxPopuli commented Jan 2, 2019

NullVoxPopuli commented Jan 2, 2019

buschtoens commented Jan 2, 2019

buschtoens commented Jan 7, 2019

buschtoens commented Jan 10, 2019

rwjblue commented Jan 10, 2019

buschtoens commented Jan 10, 2019

rwjblue commented Jan 10, 2019

rwjblue commented Jan 10, 2019

buschtoens Dec 11, 2018 •

edited