Replace URI parse with Addressable gem

[lucke84]

PR related to issue #403

Also, adds the fragment of the requested URI as a attribute of the Rails context.

---

This change is

[coveralls]

Coverage remained the same at 85.325% when pulling 332081e on InnovativeTravel:replace_uri_parse_with_addressable into 557cb3e on shakacode:master.

[justin808]

Couple comments.

---

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Gemfile, line 10 [r1] (raw file):
I believe we need to modify the gemspec for a peer dependency.

Did you try to make a new app and see if it works?

We certainly don't want apps that use ReactOnRails to have include this gem.

---

app/helpers/react_on_rails_helper.rb, line 342 [r1] (raw file):
We need a clear comment in the code as to why we are not using the standard URI.

What problem does this solve.

Otherwise, this will be confusing to somebody later on.

We can refer to the github issue.

---

Comments from Reviewable

[justin808]

@lucke84 Have you looked into why CI is failing? Maybe rebase on top of master and push to start CI again.

[coveralls]

Coverage remained the same at 85.325% when pulling a6fdbcc on InnovativeTravel:replace_uri_parse_with_addressable into 9995619 on shakacode:master.

[lucke84]

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.

---

app/helpers/react_on_rails_helper.rb, line 342 [r1] (raw file):
Done.

---

Gemfile, line 10 [r1] (raw file):
Done.

---

Comments from Reviewable

[jbhatab]

Reviewed 2 of 3 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[jbhatab]

@justin808 looks good to me if you want to merge it in.

[justin808]

Another comment on the addition of the "hash".

---

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

app/helpers/react_on_rails_helper.rb, line 355 [r3] (raw file):
This is a nice addition, actually superb.

however, this needs to be:

1. Match Location api for browser: https://developer.mozilla.org/en-US/docs/Web/API/Location (use Location.hash)
2. Update docs.
3. Provide test of this addition (see related ones).
4. Update PR description
5. Update CHANGELOG with this one. I'll revise the release and date. I'm going to do this with all PR requests.

---

Comments from Reviewable

[lucke84]

Thanks for the comments, I'd applied some changes (see specific comments).

---

Review status: 1 of 7 files reviewed at latest revision, 1 unresolved discussion.

---

app/helpers/react_on_rails_helper.rb, line 355 [r3] (raw file):
I've updated the docs (README and CHANGELOG) and added tests. The PR description already includes the fact that the fragment is included, we can maybe specify it better in the changelog (should I do it or will you?).

I'm not 100% sure what you meant with point 1, do you want the attribute to be called "hash" instead of "fragment"?

BTW, the way we populate the "href" attribute is misleading, as we use request.original_url which does not include the hash/fragment. I added it to the "location" attribute but I'm not sure it's good to leave "href" like that.

---

Comments from Reviewable

[coveralls]

Coverage remained the same at 85.325% when pulling e2278a0 on InnovativeTravel:replace_uri_parse_with_addressable into 9995619 on shakacode:master.

[lucke84]

I removed the fragment part at all: Addressable is able to recognize it but the fragment is never sent to the server so there's no point in try to read it.

[coveralls]

Coverage remained the same at 85.325% when pulling a4d95eb on InnovativeTravel:replace_uri_parse_with_addressable into 9995619 on shakacode:master.

[justin808]

---

Reviewed 1 of 3 files at r2, 1 of 2 files at r4, 1 of 2 files at r5, 1 of 4 files at r8.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[justin808]

Thanks @lucke84. Nice job!

[lucke84]

Hey @justin808, I was wondering if there's a simple way for you guys to release a new version of the 5.x.y series to include this change. According to the changelog, this change has been added to the 6.x.y series but I don't think you should be forced to upgrade in order to get it (it doesn't seem to be related whatsoever to the breaking changes of version 6, looks to me more like a minor/patch release). What do you think?

[justin808]

@lucke84 Most of the changes concern the generator and a tiny bit with the running of tests, as well as some additional bits like yours.

The v6 change is because we have a couple additional config params for the test running.

I'll send you a PR of me updating the https://github.com/shakacode/react-webpack-rails-tutorial/ and if you still feel that this is too major let me know. Give me a few hours.

[lucke84]

Sure, take your time 👍 I was just pointing out that those bits and pieces which are not part of bumping a major should have probably been released as minor/patch, hopefully useful suggestion for the next time :)

[justin808]

@lucke84 We're following full semantic versioning being pushed in the JS community. In that, ANY deprecation or addition of something that needs to change requires a major version bump.