adopted-ember-addons / ember-moment

MIT License
399 stars 122 forks source link

Space in date format throws error in 7.1.0 #211

Closed jrjohnson closed 7 years ago

jrjohnson commented 7 years ago

My tests are failing when updating to v7.1.0 seems to be an issue with spaces in the date format:

Works:

export default Ember.Controller.extend({
  today: new Date(),
  niceToday: format('today', 'MMDDYYYY')
});

Fails:

export default Ember.Controller.extend({
  today: new Date(),
  niceToday: format('today', 'MM DD YYYY')
});

The failing version spits out the error: Brace expanded properties cannot contain spaces, e.g. "user.{firstName, lastName}"

I wasn't able to get ember-moment to install in a twiddle, but this is pretty easy to reproduce. I can submit a failing test PR if desired. Just need some guidance on where it belongs - can I put it in the smoke tests?

jasonmit commented 7 years ago

7.0.1 was a bad release, are you still seeing issues on 7.1.0.

Edit: I actually think you meant 7.1.0 because it feels like the change that went into 7.1.0 that might have caused it.

/cc @kellyselden who might know more. I can spend time tonight debugging otherwise.

kellyselden commented 7 years ago

I think we should revert my change until we can figure this out. The string literal is treated as a key first, causing it to go through the property expansion algorithm. I'm surprised none of the tests had spaces to catch this.

@jrjohnson We would love a failing test. It could go here https://github.com/stefanpenner/ember-moment/blob/master/tests/unit/computeds/format-test.js, and could be called "handles string literal formats with spaces in them".

jasonmit commented 7 years ago

@kellyselden good call. I just reverted your commits, wrote a test to cover this in the future, and versioned 7.1.1.

jasonmit commented 7 years ago

Closing as this is resolved in 7.1.1, we'll open a new issue for tracking adding back the computed helpers.

jrjohnson commented 7 years ago

I wasn't seeing a failure when adding the test into format-test. I assume because the actual CP goes through layers, but the unit test just runs against the function itself. That is pure speculation, but it is where I gave up.

jasonmit commented 7 years ago

@jrjohnson master is fixed and I wrote a test to cover this in the future.

Reverting the revert and running the tests currently should fail:

not ok 68 PhantomJS 2.1 - controller:test-subject: can contain spaces in the format argument
    ---
        actual: >
            null
        stack: >
            http://localhost:7357/assets/vendor.js:23019
        message: >
            Died on test #1 testWrapper@http://localhost:7357/assets/test-support.js:7165:16
            test@http://localhost:7357/assets/test-support.js:7179:44
            http://localhost:7357/assets/tests.js:654:24
            exports@http://localhost:7357/assets/vendor.js:132:37
            requireModule@http://localhost:7357/assets/vendor.js:32:25
            require@http://localhost:7357/assets/test-support.js:6945:14
            loadModules@http://localhost:7357/assets/test-support.js:6937:21
            load@http://localhost:7357/assets/test-support.js:6967:33
            http://localhost:7357/assets/test-support.js:6850:22: Assertion Failed: Brace expanded properties cannot contain spaces, e.g. "user.{firstName, lastName}" should be "user.{firstName,lastName}"
        Log: |
    ...
jrjohnson commented 7 years ago

Hmm... Wonder why I was getting a failure there. Doesn't matter - just wanted to make sure you were so tests would actually cover the issue. Thanks for the quick response to this.