compose urls for relative paths cases #64

elangobharathi · 2018-10-05T08:41:37Z

Why

To compose relative urls explicitly using no forward slash in path value. Refer #29 for more background.

Outcome

If the path starts with forward slash, it is considered as absolute url. If not, it is considered as relative url.
For example,

base = "/people" and path = "/absolute" resolves to "/absolute"

whereas,

base = "/people" and path = "relative" resolves to "/people/relative"

Note

In the nested cases, the immediate parent's url becomes base for composition.
For example,

 <RestfulProvider base="https://my-awesome-api.fake/MY_SUBROUTE">
  <Get path="/absolute-1">
    {() => (
      <Get path="relative-1">
        {() => (
          <Get path="/absolute-2">
            {() => <Get path="relative-2">{() => <Get path="relative-3">{children}</Get>}</Get>}
          </Get>
        )}
      </Get>
    )}
  </Get>
</RestfulProvider>

emits
"https://my-awesome-api.fake/MY_SUBROUTE/absolute-1"
"https://my-awesome-api.fake/MY_SUBROUTE/absolute-1/relative-1"
"https://my-awesome-api.fake/MY_SUBROUTE/absolute-2"
"https://my-awesome-api.fake/MY_SUBROUTE/absolute-2/relative-2"
"https://my-awesome-api.fake/MY_SUBROUTE/absolute-2/relative-2/relative-3"

TejasQ

@elangobharathi awesome! Welcome to the project! Great to see your contribution!

I had some thoughts here that I'd like to request changes on, but besides that, great work!

TejasQ · 2018-10-05T08:57:35Z

src/Get.tsx

+     * If not, it is considered as relative url.
+     * For example,
+     * base = "http://example.com/people" and path = "/absolute"
+     * reolves to "http://example.com/absolute"


reolves -> resolves.

Corrected. Thank you!

TejasQ · 2018-10-05T09:00:13Z

src/Get.tsx

+     * base = "http://example.com/people" and path = "relative"
+     * reolves to "http://example.com/people/relative"
+     */
+    const composedUrl = requestPath


This set of nested ternaries is so rad, but maybe we can refactor this a bit in order to preserve readability so that when we look at it in a few months we'll understand it faster.

Ya, I understand. I have now refactored. Thanks!

TejasQ · 2018-10-05T09:00:44Z

src/Mutate.tsx

@@ -122,8 +122,12 @@ class ContextlessMutate<TData, TError> extends React.Component<

    const requestPath =
      verb === "DELETE" && typeof body === "string"
-        ? url.resolve(base!, url.resolve(path, body))
-        : url.resolve(base!, path);
+        ? path.startsWith("/")


See the other comment about nested ternaries. This also appears to be duplicated logic so maybe put it in a function that is unit tested?

It's a great idea to put that in a function. I have moved it to a util function. Thank you!

fabien0102

Nice, but we can do better! I like the idea of a util function to composeUrl, but I think that we can go a bit more deeper to avoid any url.resolve outside of this util function.

Also don't forgot to add some tests to Mutate and Poll, even if it should almost be the same as Get, we also have some subtilities over there 😉

fabien0102 · 2018-10-05T13:01:41Z

src/Mutate.tsx

-      verb === "DELETE" && typeof body === "string"
-        ? url.resolve(base!, url.resolve(path, body))
-        : url.resolve(base!, path);
+    let requestPath: string;


I think that you can keep the ternary here, for 2 choices is fine 😃

Changed it back to ternary 😃

fabien0102 · 2018-10-05T13:03:25Z

src/Mutate.tsx

+        <RestfulReactProvider
+          {...contextProps}
+          base={
+            props.path.startsWith("/")


I don't really check how url.resolve works, but this should do the stuff for the both cases.

fabien0102 · 2018-10-05T13:05:43Z

src/util/composeUrl.test.ts

+import { composeUrl } from "./composeUrl";
+
+afterEach(() => {
+  cleanup();


Don't need to cleanup here, we don't have any react implementation

I got you. Removed it. Thanks.

fabien0102 · 2018-10-05T13:06:46Z

src/util/composeUrl.test.ts

+
+describe("compose url", () => {
+  it("should provide absolute url", () => {
+    const base = "https://my-awesome-api.fake/people";


Can you add a test with a trailing slash?

Added now. Thanks.

fabien0102 · 2018-10-05T13:07:30Z

src/Poll.tsx

@@ -209,7 +210,7 @@ class ContextlessPoll<TData, TError> extends React.Component<
    const { lastPollIndex } = this.state;
    const requestOptions = this.getRequestOptions();

-    const request = new Request(url.resolve(base!, path), {
+    const request = new Request(path.startsWith("/") ? url.resolve(base!, path) : `${base!}/${path}`, {


This can be replace by composeUrl

Yes, replaced it now. Thanks

fabien0102 · 2018-10-05T13:11:26Z

src/Get.tsx

+        <RestfulReactProvider
+          {...contextProps}
+          base={
+            props.path.startsWith("/")


This can be replace by composeUrl

Yes, replaced it now. Thanks

fabien0102 · 2018-10-05T13:27:32Z

src/Get.test.tsx

+          </Get>
+        </RestfulProvider>,
+      );
+      await wait(() => expect(children.mock.calls.length).toBe(6));


Can you also add some other expects with some data from nock to be sure that we have that we are expected here? I want to know if the .get("/absolute-2/relative-2/relative-3") is well called

Yes, added them now. Thank you.

fabien0102 · 2018-10-05T13:47:41Z

After checking how url.resolve works a bit closer, you are just lucky in your test cases.

url.resolve('http://test.com/one/two/three', '/four'); // http://test.com/four
url.resolve('http://test.com/one/two/three', 'four'): // http://test.com/one/two/three/four

I was wondering how it was working, and now I finally understand 😄

To provide a real implementation of this feature, you need to add some tests with some base more complicated like this:

render(
    <RestfulProvider base="https://my-awesome-api.fake/absolute-1">
      <Get path="">
        {() => (
          <Get path="relative-1">
            {() => (
              <Get path="/absolute-2">
                {() => <Get path="relative-2">{() => <Get path="relative-3">{children}</Get>}</Get>}
              </Get>
            )}
          </Get>
        )}
      </Get>
    </RestfulProvider>
)

and also some trailing slashes. I think that url.resolve can be a very helping piece in this feature, you "just" need to store the rootBase somewhere.

TejasQ · 2018-10-05T13:56:59Z

Great catch, @fabien0102!

@elangobharathi, apologies, we may have not been as clear in describing the issue: the idea is that the base prop is the absolute root of any nested Get components whose paths are prefixed with a /, even if the base prop itself contains subpaths.

The current implementation always rolls up to the domain-level instead of a subpath on the base prop.

micha-f · 2018-10-05T13:59:51Z

I found this a while ago - CannerCMS@978a686

These changes were only made in their local branch.

elangobharathi · 2018-10-05T14:35:53Z

@TejasQ I am sorry I couldn't fully understand. Could you please explain me this with an example?

@elangobharathi, apologies, we may have not been as clear in describing the issue: the idea is that the base prop is the absolute root of any nested Get components whose paths are prefixed with a /, even if the base prop itself contains subpaths.

The current implementation always rolls up to the domain-level instead of a subpath on the base prop.

elangobharathi · 2018-10-05T14:42:43Z

@fabien0102 Thank you for the comments :) It would be great if you can provide the expected result for your example test case. This would help me understand the expectations a bit more.

render(
    <RestfulProvider base="https://my-awesome-api.fake/absolute-1">
      <Get path="">
        {() => (
          <Get path="relative-1">
            {() => (
              <Get path="/absolute-2">
                {() => <Get path="relative-2">{() => <Get path="relative-3">{children}</Get>}</Get>}
              </Get>
            )}
          </Get>
        )}
      </Get>
    </RestfulProvider>
)

TejasQ · 2018-10-05T14:43:39Z

Absolutely!

Here, let's use your PR description with some adjustments in order to make it a little more understandable:

If the path starts with forward slash, it is considered as absolute url. If not, it is considered as relative url.

^ this is correct.

For example,

base = "http://example.com/people" and path = "/absolute" 
resolves to "http://example.com/absolute"

^ this is incorrect.

Instead,

base = "http://example.com/people" and path = "/absolute" 
resolves to "http://example.com/people/absolute"

because base is /people!

whereas,

base = "http://example.com/people" and path = "relative" 
resolves to "http://example.com/people/relative"

^ Correct.

Note

In the nested cases, the immediate parent's url becomes base for composition.
For example,

 <RestfulProvider base="https://my-awesome-api.fake/MY_SUBROUTE">
  <Get path="/absolute-1">
    {() => (
      <Get path="relative-1">
        {() => (
          <Get path="/absolute-2">
            {() => <Get path="relative-2">{() => <Get path="/not-relative-3">{children}</Get>}</Get>}
          </Get>
        )}
      </Get>
    )}
  </Get>
</RestfulProvider>

emits
"https://my-awesome-api.fake/MY_SUBROUTE/absolute-1"
"https://my-awesome-api.fake/MY_SUBROUTE/absolute-1/relative-1"
"https://my-awesome-api.fake/MY_SUBROUTE/absolute-2"
"https://my-awesome-api.fake/MY_SUBROUTE/absolute-2/relative-2"
"https://my-awesome-api.fake/MY_SUBROUTE/not-relative-3"

Does that clear things up? Let me know if I can clarify further. 😄

fabien0102 · 2018-10-05T14:48:01Z

@TejasQ was quicker than me 🥇 @elangobharathi is it clear for you or do you want some test cases?

elangobharathi · 2018-10-05T14:58:05Z

@TejasQ That cleared up the base with subpath case https://my-awesome-api.fake/MY_SUBROUTE/. Thank you.

@fabien0102 @TejasQ I would definitely need the expected result for some complicated nesting cases.

Can you provide the expected result for the following cases?

render(
    <RestfulProvider base="https://my-awesome-api.fake/absolute-1">
      <Get path="">
        {() => (
          <Get path="relative-1">
            {() => (
              <Get path="/absolute-2">
                {() => <Get path="relative-2">{() => <Get path="relative-3">{children}</Get>}</Get>}
              </Get>
            )}
          </Get>
        )}
      </Get>
    </RestfulProvider>
)

render(
    <RestfulProvider base="https://my-awesome-api.fake/absolute-1">
      <Get path="/">
        {() => (
          <Get path="relative-1">
            {() => (
              <Get path="">
                {() => <Get path="relative-2">{() => <Get path="relative-3">{children}</Get>}</Get>}
              </Get>
            )}
          </Get>
        )}
      </Get>
    </RestfulProvider>
)

fabien0102 · 2018-10-05T15:32:47Z

If I don't do any mistake, this should result like this:

    it("should compose more nested absolute and relative urls", async () => {
      nock("https://my-awesome-api.fake")
        .get("/absolute-1")
        .reply(200);

      nock("https://my-awesome-api.fake")
        .get("/absolute-1/relative-1")
        .reply(200);

      nock("https://my-awesome-api.fake")
        .get("absolute-1/absolute-2")
        .reply(200);

      nock("https://my-awesome-api.fake")
        .get("absolute-1/absolute-2/relative-2")
        .reply(200);

      nock("https://my-awesome-api.fake")
        .get("absolute-1/absolute-2/relative-2/relative-3")
        .reply(200, { path: "absolute-1/absolute-2/relative-2/relative-3" });

      const children = jest.fn();
      children.mockReturnValue(<div />);

      render(
        <RestfulProvider base="https://my-awesome-api.fake/absolute-1">
          <Get path="">
            {() => (
              <Get path="relative-1">
                {() => (
                  <Get path="/absolute-2">
                    {() => <Get path="relative-2">{() => <Get path="relative-3">{children}</Get>}</Get>}
                  </Get>
                )}
              </Get>
            )}
          </Get>
        </RestfulProvider>,
      );
      await wait(() => expect(children.mock.calls.length).toBe(6));
      expect(children.mock.calls[5][0]).toEqual({ path: "absolute-1/absolute-2/relative-2/relative-3" });
    });

And same for Get[0].path="/" or RestfulProvider.base = "https://my-awesome-api.fake/absolute-1/"

Note: I'm not totally sure abouth the /absolute2 composition, let me ask to @TejasQ to have a second opinion 😉

elangobharathi · 2018-10-08T07:01:45Z

@fabien0102 Thank you for the details 😊.

@TejasQ I am waiting for your opinion.

Note: I'm not totally sure abouth the /absolute2 composition, let me ask to @TejasQ to have a >second opinion 😉

TejasQ · 2018-10-08T14:22:27Z

He asked me! He is 💯 % correct. :)

elangobharathi · 2018-10-09T11:28:31Z

@fabien0102 @TejasQ Thank you for your comments. I have addressed them now. Please review and let me know your feedback 😊

TejasQ

Thanks, @elangobharathi! I really like the approach. I'm wondering if there's a way we can more cleanly express (in terms of naming), the difference between baseRoot and base?

If I was using the library as a newcomer, I could find these two concepts fairly confusing.

elangobharathi · 2018-10-16T07:38:48Z

Hi @TejasQ Yes, you are right. We should definitely rename them. How about naming them as base and parentPath?

For example,

<RestfulProvider base="https://my-awesome-api.fake/MY_SUBROUTE">
  <Get path="/absolute-1">
    {() => (
      <Get path="relative-1">
      </Get>
    )}
  </Get>
</RestfulProvider>

While composing https://my-awesome-api.fake/MY_SUBROUTE/absolute-1/relative-1,

https://my-awesome-api.fake/MY_SUBROUTE remains base
/absolute-1 as parenthPath
relative-1 remains path

Please let me know if you can think of any better names as well 😄

TejasQ · 2018-10-16T10:01:52Z

I love that naming! Great idea!

elangobharathi · 2018-10-16T18:26:36Z

@TejasQ Thank you. Renaming is done 😊

TejasQ

Looks good to me. Let's see what @fabien0102 has to say about it. 🙂

fabien0102

Sorry for the delay, I have totally missed this notification 😕

This looks awesome 🎉 Well tested and functionnal, nothing to say, very nice job 💯

elangobharathi · 2018-10-19T13:40:15Z

@fabien0102 @TejasQ Thank you for the approvals 😊

TejasQ · 2018-10-19T14:07:05Z

So @elangobharathi if you can rebase it, we're good to go! WOOOO! THANK YOU FOR YOUR CONTRIBUTION! 🚀

elangobharathi · 2018-10-24T10:57:07Z

@TejasQ Thank you! I have just rebased it. Please check and merge. 😊

TejasQ · 2018-10-24T11:39:58Z

Thanks, Elango! It was a true pleasure working with you!

elangobharathi · 2018-10-24T12:14:20Z

Thanks @TejasQ ! It was a pleasure working with you too!

elangobharathi mentioned this pull request Oct 5, 2018

Improve clarity of backend route composition #29

Closed

TejasQ suggested changes Oct 5, 2018

View reviewed changes

fabien0102 suggested changes Oct 5, 2018

View reviewed changes

fabien0102 reviewed Oct 5, 2018

View reviewed changes

TejasQ suggested changes Oct 15, 2018

View reviewed changes

TejasQ reviewed Oct 17, 2018

View reviewed changes

fabien0102 previously approved these changes Oct 19, 2018

View reviewed changes

TejasQ previously approved these changes Oct 19, 2018

View reviewed changes

relative and absolute url composition

431abc5

elangobharathi dismissed stale reviews from TejasQ and fabien0102 via 431abc5 October 24, 2018 10:52

elangobharathi force-pushed the compose-url branch from ebfa04a to 431abc5 Compare October 24, 2018 10:52

TejasQ approved these changes Oct 24, 2018

View reviewed changes

TejasQ merged commit 5533b65 into contiamo:master Oct 24, 2018

compose urls for relative paths cases #64

compose urls for relative paths cases #64

Conversation

elangobharathi commented Oct 5, 2018 • edited Loading

Why

Outcome

Note

TejasQ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elangobharathi Oct 5, 2018 • edited Loading

Choose a reason for hiding this comment

fabien0102 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elangobharathi Oct 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabien0102 commented Oct 5, 2018

TejasQ commented Oct 5, 2018

micha-f commented Oct 5, 2018

elangobharathi commented Oct 5, 2018

elangobharathi commented Oct 5, 2018

TejasQ commented Oct 5, 2018

Note

fabien0102 commented Oct 5, 2018

elangobharathi commented Oct 5, 2018

fabien0102 commented Oct 5, 2018 • edited Loading

elangobharathi commented Oct 8, 2018

TejasQ commented Oct 8, 2018

elangobharathi commented Oct 9, 2018

TejasQ left a comment

Choose a reason for hiding this comment

elangobharathi commented Oct 16, 2018 • edited Loading

TejasQ commented Oct 16, 2018

elangobharathi commented Oct 16, 2018

TejasQ left a comment

Choose a reason for hiding this comment

fabien0102 left a comment

Choose a reason for hiding this comment

elangobharathi commented Oct 19, 2018

TejasQ commented Oct 19, 2018

elangobharathi commented Oct 24, 2018

TejasQ commented Oct 24, 2018

elangobharathi commented Oct 24, 2018

elangobharathi commented Oct 5, 2018 •

edited

Loading

elangobharathi Oct 5, 2018 •

edited

Loading

elangobharathi Oct 9, 2018 •

edited

Loading

fabien0102 commented Oct 5, 2018 •

edited

Loading

elangobharathi commented Oct 16, 2018 •

edited

Loading