search

AmpersandJS / ampersand-state

Core state management object.
MIT License
141 stars 75 forks source link

Making it possible to State.set('anyChild', null) again #225

Closed andrefarzat closed 8 years ago

andrefarzat commented 8 years ago

With the 4.9.0, the change in this line broke the behaviour of being possible to pass null or undefined to as a child.

I dunno if this is the desired behaviour, however, it was already possible before, so it would be nice keeping it as is :smile_cat:

wraithgar commented 8 years ago

Thanks for making this PR. There is an issue discussing this already here https://github.com/AmpersandJS/ampersand-state/issues/224

The suggestion there is slightly different from the implementation you took here, which I don't think is a big deal, there are likely a few ways to address this. I'll let @dhritzkiv weigh in if they want.

I'm not sure why the CI tests are failing, the saucelabs credentials are in fact in this PR

dhritzkiv commented 8 years ago

I can answer why the tests are failing: phantomjs was finally updated to 2.12, and 1.9.x was end-of-life'd. So, phantomjs dep needs to be updated (in most ampersand packages :astonished:)

STRML commented 8 years ago

It's not this?

Error:
Zuul tried to run tests in saucelabs, however no saucelabs credentials were provided.
See the zuul wiki (https://github.com/defunctzombie/zuul/wiki/Cloud-testing) for info on how to setup cloud testing.
STRML commented 8 years ago

Appears to be https://github.com/defunctzombie/zuul/issues/86

wraithgar commented 8 years ago

Aaaargh software.

STRML commented 8 years ago

We'll have to drop the saucelabs part of the test, or put the keys in plaintext. Both options are unfortunate.

wraithgar commented 8 years ago

Well I ran this PR's tests in saucelabls manually and they work, for the purposes of this PR.

dhritzkiv commented 8 years ago

Oh weird. I was getting the ELIFECYCLE error when running npm install locally, where it was complaining about phantomjs. Updating phantomjs fixed the npm error… Guess it was unrelated.

Aaaargh software.

agreed

wraithgar commented 8 years ago

+1 on this, hoping for a quick turnaround and we'll deal with testing later.

wraithgar commented 8 years ago

I know how to fix the tests. You can set env variables in travis per repo.

wraithgar commented 8 years ago

tests passed on master after merge.

published as v4.9.1

Thanks again @andrefarzat

andrefarzat commented 8 years ago

wow. You guys were fast. Thanks a lot !