fix(component): `@layout` accept template factory #451

buschtoens · 2019-07-01T12:41:18Z

This PR changed how templates get transpiled under the hood.
The export from templates are now factory functions which expect to be passed the owner.

emberjs/ember.js#18096 This PR changed how templates get transpiled under the hood. The export from templates are now factory functions which expect to be passed the `owner`.

rwjblue · 2019-07-02T20:42:46Z

CI is failing due to using Node 6:

@ember-data/-build-infra/node_modules/broccoli-rollup/dist/index.js:31
    async build() {
          ^^^^^

buschtoens · 2019-07-03T09:06:16Z

Should we pin the Ember Data dependency for now and do a major release later that drops Node 6?

emberjs/data#6048 The linked PR dropped support for Node v6 and was released a `3.11.0`. To fix our CI we pin `ember-data@~3.10.0`. We will later drop support for Node v6 ourself, but this will likely require a major release.

This release includes workmanw/ember-qunit-assert-helpers@cd6dca6

buschtoens · 2019-07-04T08:08:10Z

packages/component/addon/index.js

-    (() => typeof template === 'object' && typeof template.indexOf === 'undefined')()
+    (() =>
+      typeof template === 'function' ||
+      (typeof template === 'object' && typeof template.indexOf === 'undefined'))()


btw is there any reason this is an IIFE? If not, I'd remove that in this PR as well.

No not particularly. I'd only use an IIFE in order to simplify the boolean logic, e.g.:

assert( 'whatever', (() => { if (condition) { return true; } if (otherThing) { return true; } return false; })()

It would probably make this specific case easier to think about, but isn't a giant deal.

buschtoens · 2019-07-04T09:29:29Z

I am sure that the typescript@next error is caused by microsoft/TypeScript#30790.

Error: Errors in typescript@next for external dependencies:
../../../node_modules/@types/node/index.d.ts(187,11): error TS2300: Duplicate identifier 'IteratorResult'.
../../../node_modules/dtslint/typescript-installs/next/node_modules/typescript/lib/lib.es2015.iterable.d.ts(41,6): error TS2300: Duplicate identifier 'IteratorResult'.

I reported it here: microsoft/TypeScript#30790 (comment)

microsoft/TypeScript#30790 (comment)

In hopes of making `ember-cli-sass` inside of `ember-cli-addon-docs` work. 😭

This fixes `Uncaught TypeError: Cannot read property default of undefined`.

buschtoens · 2019-07-04T15:54:46Z

Oh come on FFS, how hard can it be to get a freaking one-line change merged? 😂

ember-cli-addon-docs and all the dependencies it brings in are just causing sooo many issues. I'll open another PR to get master clean first, when I can find some time.

buschtoens · 2019-07-05T16:18:48Z

Alright, I discovered another issue with emberjs/ember.js#18096 in combination with @layout in production builds. It's a reprise of #327.

In development builds or builds that only target browsers that support arrow functions, the returned TemplateFactory and thus first argument passed to @layout is an arrow function, e.g. owner => { ... }.

Importantly, arrow functions have no prototype key, while regular functions (function(owner) { ... }) do.

ember-decorators/packages/utils/addon/-private/class-field-descriptor.js

Lines 1 to 10 in 9f6af93

    
           function isClassDescriptor(possibleDesc) { 
        
             let [target] = possibleDesc; 
        
             return ( 
        
               possibleDesc.length === 1 && 
        
               typeof target === 'function' && 
        
               'prototype' in target && 
        
               !target.__isComputedDecorator 
        
             ); 
        
           }

This causes the above code to report a false positive in production builds an lets the @layout decorator fail with:

Error: Assertion Failed: The @layout decorator requires parameters

Since decoratorWithRequiredParams thinks the decorator is invoked like:

@layout
class {}

// instead of
@layout(template)
class {}

I suggest we stop using decoratorWithRequiredParams for @layout. Unfortunately we use a check here then, but I don't see any other way to fix this.

rwjblue · 2019-07-05T16:55:21Z

Ya, that seems fine to me...

josemarluedke · 2019-07-31T03:12:38Z

@buschtoens @rwjblue Is there anything that I can help with to move this forward? I'm currently blocked on upgrading Ember Canary because of the issue this solves.

rwjblue · 2019-08-01T14:50:27Z

AFAIK the blocker is just implementing the change @buschtoens suggested

buschtoens · 2019-08-01T14:58:31Z

I fixed the decoratorWithRequiredParams usage and removed the IIFE, but I expect CI to still fail spectacularly, because of all the other unrelated issues 🙈

rwjblue · 2019-08-01T15:56:32Z

@pzuraq - Thoughts on the path forward here?

pzuraq · 2019-08-01T16:08:20Z

I think the solution looks good, I'm not actually sure what the test failures on Canary are, it looks like they're coming from pretty deep inside the VM O_o

Also unsure what's going on with production tests. I can try to take a look sometime next week, going to be busy for the remainder of this week. I also think we could make canary tests optional at this point, since we don't expect much to be changing with regards to the object model in the near future. Ideally we wouldn't, but we could to get unblocked.

buschtoens · 2019-08-01T16:28:48Z

The canary failure is caused by emberjs/ember-test-helpers#677

Edit: I might be mistaken, but I think this was the error I set out to fix with above PR.

rwjblue · 2019-08-02T00:19:33Z

Ya, could be but I thought I fixed those issues in ember so it didn't require changes in addons (e.g. didn't need to upgrade @ember/test-helpers).

josemarluedke · 2019-08-06T15:35:54Z

Failures are agast "canary" and "production", could it be that when the tests ran, the build didn't pick up the changes that @rwjblue is referring as fixed?

Error: Cannot read property 'lookup' of undefined

The error does seem to be related, maybe some can restart the tests to see if it passes now?

Looking forward to having this fixed.

rwjblue · 2019-08-06T16:54:53Z

The production build failure was not related to the lookup failure (it was Error: Browser failed to connect within 30s. testem.js not loaded?).

Apparently my fix in ember-source only fixed setupRenderingTest style tests, not moduleForComponent. I haven't had a chance to dig into that yet though. It is blocking a couple legit bug fixes over in @ember/test-helpers though.

rwjblue · 2019-08-06T16:55:40Z

A plausible path forward here would be to run the codemod to migrate to setupRenderingTest to work around the bug in moduleForComponent.

rwjblue · 2019-08-09T16:51:58Z

OK, now ready for a rebase...

rwjblue · 2019-08-12T18:59:06Z

Rebased and merged in #454

fix(component): @layout accept template factory

816c00c

emberjs/ember.js#18096 This PR changed how templates get transpiled under the hood. The export from templates are now factory functions which expect to be passed the `owner`.

rwjblue approved these changes Jul 2, 2019

View reviewed changes

fix(deps): pin ember-data@~3.10.0

4d4d7ae

emberjs/data#6048 The linked PR dropped support for Node v6 and was released a `3.11.0`. To fix our CI we pin `ember-data@~3.10.0`. We will later drop support for Node v6 ourself, but this will likely require a major release.

buschtoens force-pushed the fix/component/layout-template-factory branch from c17a45a to 727d8f7 Compare July 3, 2019 11:49

fix(deps): pin ember-cli@~3.9.0

a094cc6

buschtoens force-pushed the fix/component/layout-template-factory branch from 727d8f7 to a094cc6 Compare July 3, 2019 12:13

fix(deps): upgrade ember-qunit-assert-helpers@^0.2.2

3caed11

This release includes workmanw/ember-qunit-assert-helpers@cd6dca6

buschtoens commented Jul 4, 2019

View reviewed changes

buschtoens added 6 commits July 4, 2019 13:56

fix(deps): upgrade @types/node@^9.6.48

d2f8b57

microsoft/TypeScript#30790 (comment)

fix(deps): upgrade ember-cli-addon-docs

7c07727

fix(deps): add sass

f3247ca

In hopes of making `ember-cli-sass` inside of `ember-cli-addon-docs` work. 😭

chore(pkg): [BREAKING] drop Node 6 support

cf497f2

chore(pkg): [BREAKING] drop Node 7 support

baf288b

fix(deps): enforce ember-component-css@^0.7.4

757c6eb

This fixes `Uncaught TypeError: Cannot read property default of undefined`.

cibernox mentioned this pull request Jul 23, 2019

ember-power-select does not work on ember octane cibernox/ember-power-select#1251

Closed

refactor(component): remove superfluos IIFE

66620a1

fix(component): do not use decoratorWithRequiredParams for @layout

faec94f

buschtoens force-pushed the fix/component/layout-template-factory branch from 152262f to faec94f Compare August 1, 2019 15:45

rwjblue approved these changes Aug 1, 2019

View reviewed changes

josemarluedke mentioned this pull request Aug 6, 2019

chore: Migrate from moduleFor* syntax #453

Merged

This was referenced Aug 7, 2019

Refactor polaris-button to ES6 class smile-io/ember-polaris#358

Merged

Refactor polaris-drop-zone smile-io/ember-polaris#359

Merged

Refactor polaris-labelled smile-io/ember-polaris#361

Closed

josemarluedke mentioned this pull request Aug 12, 2019

fix(component): @layout accept template factory #454

Merged

rwjblue closed this Aug 12, 2019

rwjblue deleted the fix/component/layout-template-factory branch August 12, 2019 18:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(component): `@layout` accept template factory #451

fix(component): `@layout` accept template factory #451

buschtoens commented Jul 1, 2019 •

edited

Loading

rwjblue commented Jul 2, 2019

buschtoens commented Jul 3, 2019

buschtoens Jul 4, 2019

rwjblue Aug 1, 2019

buschtoens commented Jul 4, 2019

buschtoens commented Jul 4, 2019

buschtoens commented Jul 5, 2019

rwjblue commented Jul 5, 2019

josemarluedke commented Jul 31, 2019

rwjblue commented Aug 1, 2019

buschtoens commented Aug 1, 2019

rwjblue commented Aug 1, 2019 •

edited

Loading

pzuraq commented Aug 1, 2019

buschtoens commented Aug 1, 2019 •

edited

Loading

rwjblue commented Aug 2, 2019

josemarluedke commented Aug 6, 2019

rwjblue commented Aug 6, 2019

rwjblue commented Aug 6, 2019

rwjblue commented Aug 9, 2019

rwjblue commented Aug 12, 2019

fix(component): @layout accept template factory #451

fix(component): @layout accept template factory #451

Conversation

buschtoens commented Jul 1, 2019 • edited Loading

rwjblue commented Jul 2, 2019

buschtoens commented Jul 3, 2019

buschtoens Jul 4, 2019

Choose a reason for hiding this comment

rwjblue Aug 1, 2019

Choose a reason for hiding this comment

buschtoens commented Jul 4, 2019

buschtoens commented Jul 4, 2019

buschtoens commented Jul 5, 2019

rwjblue commented Jul 5, 2019

josemarluedke commented Jul 31, 2019

rwjblue commented Aug 1, 2019

buschtoens commented Aug 1, 2019

rwjblue commented Aug 1, 2019 • edited Loading

pzuraq commented Aug 1, 2019

buschtoens commented Aug 1, 2019 • edited Loading

rwjblue commented Aug 2, 2019

josemarluedke commented Aug 6, 2019

rwjblue commented Aug 6, 2019

rwjblue commented Aug 6, 2019

rwjblue commented Aug 9, 2019

rwjblue commented Aug 12, 2019

fix(component): `@layout` accept template factory #451

fix(component): `@layout` accept template factory #451

buschtoens commented Jul 1, 2019 •

edited

Loading

rwjblue commented Aug 1, 2019 •

edited

Loading

buschtoens commented Aug 1, 2019 •

edited

Loading