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

fix hash-based route match #264

Merged
merged 1 commit into from Nov 6, 2014
Merged

fix hash-based route match #264

merged 1 commit into from Nov 6, 2014

Conversation

longlho
Copy link
Contributor

@longlho longlho commented Nov 4, 2014

  • update dependencies
  • remove query params from path

@longlho
Copy link
Contributor Author

longlho commented Nov 4, 2014

fixes #263

@jcrugzz
Copy link
Member

jcrugzz commented Nov 4, 2014

@longlho if this does not affect any other use cases, it LGTM. Could you switch to the ~ over the ^ in the dependencies so the 0.8.x tests don't automatically break? Thanks!

@longlho
Copy link
Contributor Author

longlho commented Nov 5, 2014

yup will do :)

@longlho
Copy link
Contributor Author

longlho commented Nov 5, 2014

@jcrugzz anything else :)?

@jcrugzz
Copy link
Member

jcrugzz commented Nov 5, 2014

This LGTM. @beaugunderson this seem good to you? I havent used director client side so I just wanted to confirm there would be no other side effects.

@jcrugzz
Copy link
Member

jcrugzz commented Nov 5, 2014

actually fuck it, this does look good. @longlho can you just predefine the regex near the require calls? it will make it faster because it will be precompiled instead of interpreted each time the replace is called. Last nit :)

@longlho longlho force-pushed the master branch 2 times, most recently from 9804912 to 3f47e76 Compare November 6, 2014 00:10
- update dependencies
- remove query params from path

- extract & declare query separator
@longlho
Copy link
Contributor Author

longlho commented Nov 6, 2014

@jcrugzz just did, squashed everything too :) The codesurgeon build process is a bit tricky IMO, any reason why you guys don't use browserify for browser packaging?

@jcrugzz
Copy link
Member

jcrugzz commented Nov 6, 2014

@longlho this was formed before browserify was a thing. I dont think @indexzero would be opposed to the idea of switching to that so we can both have a build process and the ability to be used directly in browserify.

@jcrugzz
Copy link
Member

jcrugzz commented Nov 6, 2014

and thanks!

jcrugzz added a commit that referenced this pull request Nov 6, 2014
fix hash-based route match
@jcrugzz jcrugzz merged commit 68a013b into flatiron:master Nov 6, 2014
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.

None yet

2 participants