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

Location: Add 'none' location, make 'hash' the default #935

Merged
merged 6 commits into from Jun 8, 2012

Conversation

Projects
None yet
7 participants
@joliss
Contributor

joliss commented Jun 2, 2012

  • Add NoneLocation

    I am using this in my own test suite, and it's been working well. I'm
    not adding unit tests here, since the logic is trivial, and it will get
    exercised by many other tests.

  • Instantiate Ember.Location implementation from Router

    It was instantiated in App.startRouting before, instead of Router.init.

    Now we can use string locations when testing the Router in isolation,
    without requiring app.startRouting to be called.

  • Clean up location stubbing in routable_test

  • Use 'hash' as default location implementation on Router

    Even when pushState is implemented later, 'hash' will be a reasonable
    default, since pushState URL manipulation requires server-side support.

  • Router.location cannot be null or undefined

  • Avoid using HashLocation in tests

    It interacts with the browser and might be a source of brittleness.


I'm sending a batch of commits because some of the earlier cleanup is required for the test suite to still pass for later commits, so it wasn't possible to send them separately.

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jun 2, 2012

This pull request passes (merged 017dfca7 into 39bb0d0).

travisbot commented Jun 2, 2012

This pull request passes (merged 017dfca7 into 39bb0d0).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jun 3, 2012

This pull request passes (merged fdd388fe into 19cb7c9).

travisbot commented Jun 3, 2012

This pull request passes (merged fdd388fe into 19cb7c9).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jun 3, 2012

This pull request passes (merged 1841d2f4 into 90566d3).

travisbot commented Jun 3, 2012

This pull request passes (merged 1841d2f4 into 90566d3).

@jbrown

This comment has been minimized.

Show comment
Hide comment
@jbrown

jbrown Jun 4, 2012

Contributor

I'm not particularly fond of the name NoneLocation. I think MockLocation or NoopLocation are a bit more descriptive.

Contributor

jbrown commented Jun 4, 2012

I'm not particularly fond of the name NoneLocation. I think MockLocation or NoopLocation are a bit more descriptive.

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jun 4, 2012

This pull request passes (merged 126e8d49 into c233d1d).

travisbot commented Jun 4, 2012

This pull request passes (merged 126e8d49 into c233d1d).

@wagenet

This comment has been minimized.

Show comment
Hide comment
@wagenet

wagenet Jun 6, 2012

Member

@joliss looks like this doesn't merge cleanly anymore.

Member

wagenet commented Jun 6, 2012

@joliss looks like this doesn't merge cleanly anymore.

@wycats

This comment has been minimized.

Show comment
Hide comment
@wycats

wycats Jun 6, 2012

Member

Shouldn't NoneLocation trigger the onUpdateURL?

Member

wycats commented Jun 6, 2012

Shouldn't NoneLocation trigger the onUpdateURL?

@joliss

This comment has been minimized.

Show comment
Hide comment
@joliss

joliss Jun 6, 2012

Contributor

@joliss looks like this doesn't merge cleanly anymore.

Thanks, I rebased!

Shouldn't NoneLocation trigger the onUpdateURL?

The way I understand it, onUpdateURL should trigger whenever the browser changes the URL (back, forward, URL bar), right? Since NoneLocation isn't wired up to anything, I believe it should never trigger the callback.

Contributor

joliss commented Jun 6, 2012

@joliss looks like this doesn't merge cleanly anymore.

Thanks, I rebased!

Shouldn't NoneLocation trigger the onUpdateURL?

The way I understand it, onUpdateURL should trigger whenever the browser changes the URL (back, forward, URL bar), right? Since NoneLocation isn't wired up to anything, I believe it should never trigger the callback.

@joliss

This comment has been minimized.

Show comment
Hide comment
@joliss

joliss Jun 6, 2012

Contributor

Shouldn't NoneLocation trigger the onUpdateURL?

I've added a comment in the code so this doesn't trip people up when they're reading the code.

Contributor

joliss commented Jun 6, 2012

Shouldn't NoneLocation trigger the onUpdateURL?

I've added a comment in the code so this doesn't trip people up when they're reading the code.

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jun 6, 2012

This pull request passes (merged 398cf04b into 9676856).

travisbot commented Jun 6, 2012

This pull request passes (merged 398cf04b into 9676856).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jun 6, 2012

This pull request passes (merged e22de5e3 into d4e5a4a).

travisbot commented Jun 6, 2012

This pull request passes (merged e22de5e3 into d4e5a4a).

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jun 6, 2012

This pull request passes (merged e9fa6caa into d4e5a4a).

travisbot commented Jun 6, 2012

This pull request passes (merged e9fa6caa into d4e5a4a).

@wycats

This comment has been minimized.

Show comment
Hide comment
@wycats

wycats Jun 6, 2012

Member

Can you rebase this commit?

Member

wycats commented Jun 6, 2012

Can you rebase this commit?

@joliss

This comment has been minimized.

Show comment
Hide comment
@joliss

joliss Jun 6, 2012

Contributor

Can you rebase this commit?

Sure.

There are test failures, but I think they are the same as on master. (Bisect says the culprit is @digitaltoad's 2736ee7, merged in 4afbe7b.)

If master is fixed before this PR is merged, I'll rebase again to make sure everything is OK.

Contributor

joliss commented Jun 6, 2012

Can you rebase this commit?

Sure.

There are test failures, but I think they are the same as on master. (Bisect says the culprit is @digitaltoad's 2736ee7, merged in 4afbe7b.)

If master is fixed before this PR is merged, I'll rebase again to make sure everything is OK.

@digitaltoad

This comment has been minimized.

Show comment
Hide comment
@digitaltoad

digitaltoad Jun 6, 2012

Contributor

It is indeed my fault. Looks like Phantomjs does not play well with pushState.

Contributor

digitaltoad commented Jun 6, 2012

It is indeed my fault. Looks like Phantomjs does not play well with pushState.

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jun 6, 2012

This pull request passes (merged 22e4a5f3 into c392cd7).

travisbot commented Jun 6, 2012

This pull request passes (merged 22e4a5f3 into c392cd7).

@joliss

This comment has been minimized.

Show comment
Hide comment
@joliss

joliss Jun 7, 2012

Contributor

Thanks for fixing, @digitaltoad. I rebased it again, and it works now.

Contributor

joliss commented Jun 7, 2012

Thanks for fixing, @digitaltoad. I rebased it again, and it works now.

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jun 7, 2012

This pull request passes (merged d2e08e7a into be05737).

travisbot commented Jun 7, 2012

This pull request passes (merged d2e08e7a into be05737).

joliss added some commits Jun 2, 2012

Use 'hash' as default location implementation on Router
'hash' is a reasonable default over 'history' (pushState), since
pushState URL manipulation requires server-side support.
Add NoneLocation
I am using this in my own test suite, and it's been working well. I'm
not adding unit tests here, since the logic is trivial, and it will get
exercised by many other tests.
Instantiate Ember.Location implementation from Router
It was instantiated in App.startRouting before, instead of Router.init.

Now we can use string locations when testing the Router in isolation,
without requiring `app.startRouting` to be called.
Avoid using HashLocation in tests
It interacts with the browser and might be a source of brittleness.
@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Jun 8, 2012

This pull request passes (merged 50aff86 into 4f63e8d).

travisbot commented Jun 8, 2012

This pull request passes (merged 50aff86 into 4f63e8d).

wycats added a commit that referenced this pull request Jun 8, 2012

Merge pull request #935 from joliss/location
Location: Add 'none' location, make 'hash' the default

@wycats wycats merged commit 7b15a79 into emberjs:master Jun 8, 2012

@devinus

This comment has been minimized.

Show comment
Hide comment
@devinus

devinus Jun 8, 2012

Member

@joliss Although it's subjective, and without bikeshedding too much, I agree with @jbrown that maybe a better name like NullLocation or something would make more sense than none. Just my worthless 2c.

Member

devinus commented Jun 8, 2012

@joliss Although it's subjective, and without bikeshedding too much, I agree with @jbrown that maybe a better name like NullLocation or something would make more sense than none. Just my worthless 2c.

@joliss

This comment has been minimized.

Show comment
Hide comment
@joliss

joliss Jun 8, 2012

Contributor

@jbrown I think mock is technically wrong, and for whatever reason noop didn't seem quite as clear to me as none. But if you feel strongly about it, feel free to submit a pull request -- I'm not objecting.

@devinus NullLocation is tempting indeed. But then it'd be written location: 'null', which is just begging to cause confusion.

Contributor

joliss commented Jun 8, 2012

@jbrown I think mock is technically wrong, and for whatever reason noop didn't seem quite as clear to me as none. But if you feel strongly about it, feel free to submit a pull request -- I'm not objecting.

@devinus NullLocation is tempting indeed. But then it'd be written location: 'null', which is just begging to cause confusion.

@jbrown

This comment has been minimized.

Show comment
Hide comment
@jbrown

jbrown Jun 9, 2012

Contributor

@joliss In whatever app you're developing, are you using NoneLocation on your router? Or do you only use it in your test suite?

Contributor

jbrown commented Jun 9, 2012

@joliss In whatever app you're developing, are you using NoneLocation on your router? Or do you only use it in your test suite?

@joliss

This comment has been minimized.

Show comment
Hide comment
@joliss

joliss Jun 9, 2012

Contributor

Only in the test suite. (Mocha with Konacha.)

Contributor

joliss commented Jun 9, 2012

Only in the test suite. (Mocha with Konacha.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment