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

Update all the things #564

Merged
merged 14 commits into from
Feb 12, 2018
Merged

Update all the things #564

merged 14 commits into from
Feb 12, 2018

Conversation

offirgolan
Copy link
Collaborator

@offirgolan offirgolan commented Feb 10, 2018

Changes proposed:

  • Use ember-cli-update to update to the latest ember and ember-cli
  • Run codemod
  • Setup eslint + prettier and run prettier
  • Support Ember 2.12 LTS+

Todo (in different PRs)

  • Fix ember beta env issue with the descriptor changes (@rwjblue would love some input on that)
  • Get off of jquery 2.x (required by demo page which uses bootstrap3)

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me (only had a few minor inline comments below)...

LICENSE.md Outdated
@@ -1,27 +1,9 @@
Copyright 2016 Yahoo Inc.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably bring back the original here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im not sure here! Since Yahoo as an entity no longer exists, im not sure that the license still stands? If that's not the case, I'll revert it back to the BSD-3 license.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ended up reverting the license back.

@@ -1,29 +1,15 @@
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these not needed?

Copy link
Collaborator Author

@offirgolan offirgolan Feb 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. I've looked at some older yahoo OSS and they dont have it. I think just the BSD-3 license was enough.

@@ -33,7 +26,7 @@ export function unwrapProxy(o) {
}

export function isProxy(o) {
return !!(o && (o instanceof Ember.ObjectProxy || o instanceof Ember.ArrayProxy));
return !!(o && (o instanceof ObjectProxy || o instanceof ArrayProxy));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might also want to check for unknownProperty (I know this is unrelated)

@rwjblue
Copy link
Member

rwjblue commented Feb 10, 2018

Fix ember beta env issue with the descriptor changes (@rwjblue would love some input on that)

Happy to chat through it, though I'm not sure off the cuff exactly what breaks for this ATM...

@offirgolan
Copy link
Collaborator Author

offirgolan commented Feb 10, 2018

Happy to chat through it, though I'm not sure off the cuff exactly what breaks for this ATM

My concern with the new CP get rfc that landed in beta is when we try extract the dependentKeys from a CP passed to a validator.

For example when you get a validator like this:

{
  myProp: validator('presence', {
    presence: true,
    disabled: computed('model.foo', '[email protected]', function () {})
  });
}

We would extract the dependent keys from the disabled CP and add them to the myProp cp dependentKeys.

This currently happens here.

It looks like the only thing that is breaking is the test case (since its trying to do someEmberObject.someProp.isDescriptor which throws since in the future someProp would return the computed value).

Do you think the that logic we have now for extracting the dependentKeys is still okay?

@offirgolan offirgolan merged commit f6f7fc9 into master Feb 12, 2018
@offirgolan offirgolan deleted the update-fuckin-everything branch February 12, 2018 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants