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

Add an origin function #263

Merged
merged 6 commits into from
Nov 12, 2015
Merged

Conversation

justinmchase
Copy link
Contributor

The origin consists of only the scheme and authority. This change adds the following ways to work with the origin:

URI.buildOrigin(parts);
u.origin()
u.origin(uri)

Ascii art screenshot:
screen shot 2015-11-12 at 2 11 12 pm

The origin consists of only the scheme and authority. This change adds the following ways to work with the origin:

URI.buildOrigin(parts);
u.origin()
u.origin(uri)
if (v === undefined) {
return this._parts.hostname ? URI.buildOrigin(this._parts) : '';
} else {
parts = URI.parse(v);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would have used the existing accessory here as well:

var origin = URI(v);
this
  .protocol(origin.protocol())
  .authority(origin.authority())
  .build(!build);
return this;

While simpler to read, it is slower because of double parsing…

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so that's better, even with double parsing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to do this despite double building. Easier to read.

@rodneyrehm
Copy link
Member

In test/urls.js a bunch of URLs are listed and expressed in their components to verify various input. Like the resource is a compound of path, query and fragment, the origin should be added to every entry to validate the compound of protocol and authority.

@justinmchase
Copy link
Contributor Author

I updated the urls.js tests, I found one issue where the protocol is set but the authority is not. In that case the origin is also empty.

e.g.

assert.equal('', URI('http:///foo/bar.html').origin())

@rodneyrehm
Copy link
Member

looking good, thx! all that's missing now is adding the new method to the docs and having fun with some ascii art. Do you want to try that as well?

@justinmchase
Copy link
Contributor Author

I added some stuff to docs.html. I'll look at the other one now.

@justinmchase
Copy link
Contributor Author

Fixed doco verbage based on your last comment.

rodneyrehm added a commit that referenced this pull request Nov 12, 2015
feature(origin) adding origin() accessor
@rodneyrehm rodneyrehm merged commit c6c7013 into medialize:master Nov 12, 2015
@justinmchase justinmchase deleted the feature/origin branch November 12, 2015 20:52
rodneyrehm added a commit that referenced this pull request Nov 12, 2015
@rodneyrehm
Copy link
Member

thank you very much :)

@justinmchase
Copy link
Contributor Author

Thank you! We look forward to getting it in an update.

@rodneyrehm
Copy link
Member

I'll try to make that happen tomorrow…

@rodneyrehm
Copy link
Member

released in v1.17.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants