Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Support Ember 3.13.0 #662

Open
wants to merge 4 commits into
base: master
from

Conversation

@GavinJoyce
Copy link
Contributor

commented Sep 23, 2019

This PR adds support for Ember 3.13.0

Screenshot 2019-09-23 at 13 27 18

There is one issue which might need to be resolved before this is merged:

  • #642 - I'm currently skipping this test. @snewcomer, perhaps you might have context on the need for this test
@GavinJoyce GavinJoyce changed the title support Ember 3.13.0 Support Ember 3.13.0 Sep 23, 2019
@GavinJoyce GavinJoyce changed the title Support Ember 3.13.0 [WIP] Support Ember 3.13.0 Sep 23, 2019
@GavinJoyce GavinJoyce force-pushed the GavinJoyce:gj/ember-3.13 branch from 26e863f to 6dd0997 Sep 23, 2019
@@ -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 = [

This comment has been minimized.

Copy link
@efx

efx Sep 23, 2019

Contributor

if this lands we should close #653.

@offirgolan

This comment has been minimized.

Copy link
Owner

commented Sep 23, 2019

#642 - I'm currently skipping this test.

The test is related to this part of the code base:

if (
shouldCallSuper(this, VALIDATIONS_CLASS) ||
validationMixinCount > 1
) {
inheritedClass = this._super();
}

And is pretty much testing that if there is no super class with a validations mixin, it shouldnt call this._super at all. This was related to a bug that was found some time ago that @rwjblue helped solve. Pretty much, if you instantiated the validations object within an ember action, it will call this._super on that action which would invoke that action's parent method.

@offirgolan

This comment has been minimized.

Copy link
Owner

commented Sep 23, 2019

We can skip the test for now and come back for this later if it means we can merge this and unblock everyone else.

@offirgolan

This comment has been minimized.

Copy link
Owner

commented Sep 23, 2019

@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'

This comment has been minimized.

Copy link
@rwjblue

rwjblue Sep 23, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@Turbo87

Turbo87 Sep 26, 2019

Contributor

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

This comment has been minimized.

Copy link
@jrjohnson

jrjohnson Oct 14, 2019

Contributor

I went with @Turbo87's suggestion and pinned the new scenarios at 8. I'll follow up with another to drop Node 6.

'@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]);

This comment has been minimized.

Copy link
@rwjblue

rwjblue Sep 23, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@jrjohnson

jrjohnson Oct 14, 2019

Contributor

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];

This comment has been minimized.

Copy link
@rwjblue

rwjblue Sep 23, 2019

Collaborator

What scenario is altKey used for?

This comment has been minimized.

Copy link
@GavinJoyce

GavinJoyce Sep 23, 2019

Author Contributor

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 comment has been minimized.

Copy link
@balinterdi

balinterdi Sep 23, 2019

dependentKeys.filter(Boolean) ?

@Turbo87 Turbo87 referenced this pull request Sep 26, 2019
@jrjohnson

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

This is our blocker for trying out the Octane preview. Anything I can do to help move this along?

@jrjohnson jrjohnson referenced this pull request Oct 9, 2019
jrjohnson added 2 commits Oct 14, 2019
Temporary fix until node 6 support is dropped.
This is not included in Array in IE 11.
@jrjohnson jrjohnson referenced this pull request Oct 14, 2019
Addressing Code Comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.