[WIP] Support Ember 3.13.0 #662
Conversation
GavinJoyce
force-pushed
the
gj/ember-3.13
branch
from
26e863f to
6dd0997
Compare
GavinJoyce
force-pushed
the
gj/ember-3.13
branch
from
6dd0997 to
259947d
Compare
ghost
mentioned this pull request
| @@ -1,16 +1,21 @@ | |||
| import Ember from 'ember'; | |||
|
|
|||
| let __EMBER_METAL__; | |||
| if (Ember.__loader.registry['@ember/-internals/metal']) { | |||
| __EMBER_METAL__ = Ember.__loader.require('@ember/-internals/metal'); | |||
| let emberMetalPaths = [ | |||
The test is related to this part of the code base: And is pretty much testing that if there is no super class with a validations mixin, it shouldnt call |
|
We can skip the test for now and come back for this later if it means we can merge this and unblock everyone else. |
|
@GavinJoyce is this PR still WIP? |
.travis.yml
Outdated
| @@ -3,7 +3,7 @@ language: node_js | |||
| node_js: | |||
| # we recommend testing addons with the same minimum supported node version as Ember CLI | |||
| # so that your addon works for all apps | |||
| - '6' | |||
| - '10' | |||
There was a problem hiding this comment.
We should probably use 8 (minimum supported Node) and do this in a separate PR to drop Node 6 support (will be breaking). Will also need to tweak the engines.node entry in package.json.
There was a problem hiding this comment.
to get this unblocked in a non-breaking way we could also run the scenarios for Ember 3.13 and above on node_js: 10 and keep the others at node_js: 6 for now until we actually drop support
There was a problem hiding this comment.
I went with @Turbo87's suggestion and pinned the new scenarios at 8. I'll follow up with another to drop Node 6.
addon/-private/ember-internals.js
Outdated
| '@ember/-internals/metal', // ember-source from 3.10 | ||
| '@ember/-internals/metal/index' // ember-source from 3.13 | ||
| ]; | ||
| let metalPath = emberMetalPaths.find(path => Ember.__loader.registry[path]); |
There was a problem hiding this comment.
FWIW, we shouldn't use Array.prototype.find if we still intend for this library to support IE11 (I'm not sure if we do though?).
Possible future refactor would be to swap this to using ember-compatibility-helpers so we don't need iterative looping.
There was a problem hiding this comment.
Papered this over with Ember.A for now.
| } | ||
|
|
||
| export function getDependentKeys(descriptorOrDecorator) { | ||
| if (__EMBER_METAL__ && __EMBER_METAL__.descriptorForDecorator) { | ||
| let descriptor = __EMBER_METAL__.descriptorForDecorator( | ||
| descriptorOrDecorator | ||
| ); | ||
| return descriptor._dependentKeys; | ||
| return descriptor._dependentKeys || [descriptor.altKey]; |
There was a problem hiding this comment.
What scenario is altKey used for?
There was a problem hiding this comment.
I'm not sure TBH. There are failing tests without it
| @@ -633,6 +633,8 @@ function getCPDependentKeysFor(attribute, model, validations) { | |||
| dependentKeys.push('model.isDeleted'); | |||
| } | |||
|
|
|||
| dependentKeys = dependentKeys.filter(dependentKey => !!dependentKey); | |||
|
This is our blocker for trying out the Octane preview. Anything I can do to help move this along? |
Temporary fix until node 6 support is dropped.
This is not included in Array in IE 11.
Addressing Code Comments
|
Rebased in #668 |
This PR adds support for Ember 3.13.0
There is one issue which might need to be resolved before this is merged: