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

AMD support, Auto-fetch credentials from Collection/Model url #6

Merged
merged 6 commits into from Jul 22, 2013
Merged

AMD support, Auto-fetch credentials from Collection/Model url #6

merged 6 commits into from Jul 22, 2013

Conversation

ghost
Copy link

@ghost ghost commented Jul 4, 2013

  • Added AMD support
  • Automatically fetch basic auth credentials from Collection/Model url property
  • Improved library documentation

@fiznool
Copy link
Owner

fiznool commented Jul 5, 2013

Very nice. Thanks for the PR.

I wonder if there is merit in keeping the explicit set and clear functions, in case the developer wants to set BasicAuth this way. Then you have the choice of URL-based or function-based. What do you think?

@ghost
Copy link
Author

ghost commented Jul 5, 2013

Dunno, what would be the use case? Would it help based on your experience?

To me it feels like setting the url property ( or a value read by a function assigned to it ) would replace both set and clear, no extra apis to learn about. ( I was actually expecting Backbone to already have this behavior )

What do you think?

@fiznool
Copy link
Owner

fiznool commented Jul 5, 2013

Dunno, what would be the use case? Would it help based on your experience?

I'm not really sure... I think the URL based approach will fit, and thinking about it, it is the correct way of doing things, according to the Basic Auth spec. I was also thinking of keeping set and clear for backwards compatibility, but its a 0. release, so its reasonable to expect breaking changes.

Before I pull in, just wanted to check - does the code take care of removing the username:password cleartext from the URL before making the remote request? I can't see where this might be happening, but I might just be blind.

@ghost
Copy link
Author

ghost commented Jul 5, 2013

Ah, that's a good point, backwards compat, but as you say, it's not a minor release.

The credentials are stripped away by jQuery already.

Luis - @lmjabreu

On 5 Jul 2013, at 11:24, Tom Spencer notifications@github.com wrote:

Dunno, what would be the use case? Would it help based on your experience?

I'm not really sure... I think the URL based approach will fit, and thinking about it, it is the correct way of doing things, according to the Basic Auth spec. I was also thinking of keeping set and clear for backwards compatibility, but its a 0. release, so its reasonable to expect breaking changes.

Before I pull in, just wanted to check - does the code take care of removing the username:password cleartext from the URL before making the remote request? I can't see where this might be happening, but I might just be blind.


Reply to this email directly or view it on GitHub.

@fiznool fiznool merged commit 76410b9 into fiznool:master Jul 22, 2013
@fiznool
Copy link
Owner

fiznool commented Jul 22, 2013

Sorry this has taken so long, now merged in. I decided to use Backbone.sync instead of overriding the individual Collection and Model sync methods, as I quite often find myself overriding an individual Model or Collection sync method, and I would probably forget to call the superclass...

Thanks too for providing the AMD code. Please let me know if you find any issues with the newest version.

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