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

Enable Fastboot relative URL #143

Merged
merged 2 commits into from
Nov 16, 2018
Merged

Enable Fastboot relative URL #143

merged 2 commits into from
Nov 16, 2018

Conversation

xg-wang
Copy link
Member

@xg-wang xg-wang commented Nov 10, 2018

This PR overrides #128, fixes #138 #110

Implementation stolen from https://github.com/ember-fastboot/ember-cli-fastboot/blob/master/fastboot/initializers/ajax.js

Also upgrade node-fetch to 2.3, which has the abort support

TODO:

  • automatic test (or just manual test?)

@xg-wang
Copy link
Member Author

xg-wang commented Nov 10, 2018

@mike-north My local ember-cli-fastboot server doesn't really give me origin so I ended up doing the string concatenation.

@xg-wang
Copy link
Member Author

xg-wang commented Nov 10, 2018

Passed ember s. Before ember-cli-fastboot is installed it was failing because of relative URL.

@chrism
Copy link

chrism commented Nov 11, 2018

I think it should be noted that this approach doesn't seem to work for either absolute or relative URLs when using Fastboot with Prember.

By that I mean when I use the PR from @rwwagner90 #128 then using a fetch in a model hook like this:

model() {
  return fetch('/assets/some.json').then(res => {
    return res.json();
  });
}

works with Fastboot, but gives an error when using with Prember.

Error while processing route: Only HTTP(S) protocols are supported
TypeError: Only HTTP(S) protocols are supported

However, this seems to be able to be resolved by including the absolute URL (which isn't ideal but is a workable solution for the few fetch requests I make).

model() {
  let path = '/assets/some.json';

  if (this.fastboot.isFastBoot) {
    const headers = this.fastboot.request.headers;
    let host = headers.get('host');
    path = `http://${host}${path}`;
  }

  return fetch(path).then(res => {
    return res.json();
  });
}

Although this isn't ideal it does work and allows for Prember builds.

Whereas this PR does not—there is still the same error message as before even after including this code.

I hope others agree that Prember is an increasingly important use case for Fastboot and that a solution which doesn't seem to be compatible with it is inferior to one which does.

If this gets merged as is then I'm concerned that there will still be an issue for the majority of users.

Maybe the fix is needed on the Prember side but I thought it was worth making others aware of this.

@xg-wang
Copy link
Member Author

xg-wang commented Nov 12, 2018

Hi @chrism, can you share a reproduction repo?

@chrism
Copy link

chrism commented Nov 12, 2018 via email

@RobbieTheWagner
Copy link

@xg-wang @chrism my site uses prember, and you can feel free to test it there https://github.com/shipshapecode/shipshape.io

Is there a reason we cannot use the code from my PR?

@xg-wang
Copy link
Member Author

xg-wang commented Nov 12, 2018

@rwwagner90 That PR only allows relative URLs for adapters, a pure fetch(...) won't go through the adapter. So @chrism need to add

  if (this.fastboot.isFastBoot) {
    const headers = this.fastboot.request.headers;
    let host = headers.get('host');
    path = `http://${host}${path}`;
  }

before making a fetch.

@chrism Would be very helpful if you can create a small repro for your code snippet with Prember setup.

@RobbieTheWagner
Copy link

@xg-wang as I said above, please use my repo for prember testing. https://github.com/shipshapecode/shipshape.io

@xg-wang
Copy link
Member Author

xg-wang commented Nov 13, 2018

@rwwagner90 Yea I will. I want to use both your app and his for Prember testing. A minimal setup can be easier to set debugger to play around with. I can try a get one but since @chrism seems already have a repro we can just use his.

@xg-wang
Copy link
Member Author

xg-wang commented Nov 14, 2018

@rwwagner90 Do you have a branch that's breaking bc relative URL for shipshape.io?
@chrism any updates for creating a repro?

@RobbieTheWagner
Copy link

@xg-wang all prember apps should have a similar issue, I would assume. If you pull master of my repo and install the branch of ember-fetch, you should likely see the same issues @chrism was seeing.

@chrism
Copy link

chrism commented Nov 14, 2018

Hi, I’m sorry I haven’t yet pushed the example repo. It’s been quite time consuming to create the most simple illustration of where one branch works and the other doesn’t.

I’m at a conference this week but I’ll find some time on Friday, at the latest.

@xg-wang
Copy link
Member Author

xg-wang commented Nov 14, 2018

@rwwagner90 🔥

-    "ember-fetch": "rwwagner90/ember-fetch#server-side",
+    "ember-fetch": "xg-wang/ember-fetch#fastboot",
RSS Feed created successfully
sitemap.xml successfully created!
Environment: production
pre-render / 200 OK
pre-render /ember-consulting/ 200 OK
ember-svg-jar: Missing inline SVG for ember-async-await-for-each
pre-render /open-source/ 200 OK
pre-render /contact/ 200 OK
pre-render /blog/ 200 OK
pre-render /blog/author/rwwagner90/ 200 OK
pre-render /blog/ad-hoc-relationships-with-ember-data/ 200 OK
pre-render /blog/aiming-for-targets-with-ember/ 200 OK
pre-render /blog/ember-3-1-testing-and-optional-features/ 200 OK
pre-render /blog/ember-data-belongs-to-find-or-create/ 200 OK
pre-render /blog/ember-data-passing-query-params-to-save/ 200 OK
pre-render /blog/ember-in-repo-addons/ 200 OK
pre-render /blog/ember-inspector-the-journey-so-far/ 200 OK
pre-render /blog/ember-meta-tags-seo-social/ 200 OK
pre-render /blog/ember-without-jquery/ 200 OK
pre-render /blog/forcing-trailing-slashes-for-routes/ 200 OK
pre-render /blog/helpful-resources-for-new-ember-devs/ 200 OK
pre-render /blog/offline-first-prember-and-service-workers/ 200 OK
pre-render /blog/reloading-hasmany-relationships/ 200 OK
pre-render /blog/static-blogs-with-prember-and-markdown/ 200 OK
pre-render /blog/testing-ember-addons-in-an-app/ 200 OK
pre-render /blog/using-components-in-ember-mixin-unit-tests/ 200 OK
The following options have been renamed — please update your config: exports -> output.exports
The following options have been renamed — please update your config: exports -> output.exports
cleaning up...
Built project successfully. Stored in "dist/".
File sizes:
 - dist/assets/app-c4992ee616c616d1d00f12b429c2a153.js: 266.93 KB (267.03 KB gzipped)
 - dist/assets/auto-import-fastboot-d41d8cd98f00b204e9800998ecf8427e.js: 20 B (31 B gzipped)
 - dist/assets/vendor-312a49b51b2f64047010d77d00b7d4d1.js: 251.34 KB (251.44 KB gzipped)
 - dist/assets/vendor-bccffd23e896a6a8f4cb593a43b169b5.css: 4.93 KB (4.88 KB gzipped)
 - dist/assets/website-4f00cc2d75de5ff99627f1daa13825df.js: 15.67 KB (15.7 KB gzipped)
 - dist/assets/website-70eaa3e26c637630bedf5b0ff83fed5f.css: 5.48 KB (5.5 KB gzipped)
 - dist/assets/website-fastboot-5c044a60bd59af8d9d504e6d85730fff.js: 764 B (787 B gzipped)
 - dist/ember-fetch/fastboot-fetch-d0a987aa68a35560ff74884f82d87c8d.js: 462 B (485 B gzipped)
 - dist/sw.js: 2.7 KB (2.73 KB gzipped)
✨  Done in 70.05s.
>>> elapsed time 1m11s

@chrism Can you test this branch when you are free?

@RobbieTheWagner
Copy link

@xg-wang nice! Glad it's working 😄 . I can test things out and poke around the app a bit this weekend.

@chrism
Copy link

chrism commented Nov 16, 2018

Hi @xg-wang I've just used this branch instead of @rwwagner90's and it works with my project too.

👍 🎉 😄

I'll still try to finish the test repos I'm working on this weekend to show the most simple use cases as I think ideally maybe one day they could perhaps even be incorporated into some testing.

Thanks to both you and @rwwagner90 for getting this working.

define fetch module at instance-initializers to get request info
- Prember is not sending protocol, need to check "undefined:"
- node-fetch 2.3 has AbortController support
- Fix issue when fetch(request) instead of fetch(url, options)
@@ -40,6 +40,8 @@
"ember-cli": "~3.0.2",
"ember-cli-dependency-checker": "^3.0.0",
"ember-cli-htmlbars": "^3.0.1",
"ember-cli-eslint": "^4.2.1",
Copy link
Member

Choose a reason for hiding this comment

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

@xg-wang why did you add this again?

Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't be there, I must did a bad rebase.

buschtoens added a commit to buschtoens/ember-cli-addon-docs that referenced this pull request Nov 19, 2018
This upgrades ember-fetch to `^6.2.0`. The primary reason for upgrading are the fixes for:

- Usage of ember-fetch in monorepos:
  ember-cli/ember-fetch#165
  https://github.com/ember-cli/ember-fetch/releases/tag/v6.1.1

- Enabling Fastboot relative URLs:
  ember-cli/ember-fetch#143
  https://github.com/ember-cli/ember-fetch/releases/tag/v6.2.0
@xg-wang xg-wang mentioned this pull request Nov 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not working with Fastboot and ember-data
4 participants