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 Transmission support #637

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jag-k
Copy link

@jag-k jag-k commented Apr 28, 2023

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #634

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I've read & comply with the contributing guidelines
  • I have tested my code for new features & regressions on both mobile & desktop devices, using the latest version of major browsers.
  • I have made corresponding changes to the documentation (README.md).
  • I've checked my modifications for any breaking changes, especially in the config.yml file

@netlify
Copy link

netlify bot commented Apr 28, 2023

Deploy Preview for homer-demo-content ready!

Name Link
🔨 Latest commit 026ae82
🔍 Latest deploy log https://app.netlify.com/sites/homer-demo-content/deploys/66468bfc3e64900008b95009
😎 Deploy Preview https://deploy-preview-637--homer-demo-content.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jag-k jag-k changed the title ✨ Add Transmission support Add Transmission support Apr 28, 2023
Copy link
Owner

@bastienwirtz bastienwirtz left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @jag-k. A transmission custom service will be nice!
I think we can simplify a bit the data fetching part, but other than than, it should be good.

Comment on lines +122 to +130
if (this.proxy?.useCredentials) {
options.credentials = "include";
}
// Each item can override the credential settings
if (this.item.useCredentials !== undefined) {
options.credentials =
this.item.useCredentials === true ? "include" : "omit";
}
Copy link
Owner

Choose a reason for hiding this comment

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

A custom fetch function is available (you already included it using the service mixin), which handle the credential option and more. Could you update your code to use it and avoid code duplication ? Thanks 🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

@bastienwirtz

With the existing custom fetch, we don't have access to the response in the event of a non Response.OK result. We would need access to the response in order to obtain the "X-Transmission-Session-Id" after we are sent a 409 status code.

I have hacked it trivially with, but I'm sure there is a better / more correct way.

In service.js

`fetch: function (path, init, json = true, forceResponse = false)`
...
...
        if (!response.ok) {
          if (!forceResponse)
            throw new Error("Not 2xx response");
          else
            return response;
        }

Comment on lines +144 to +156
let url;
if (this.item?.rpc) {
url = this.item.rpc;
} else {
url = this.endpoint;
if (url.endsWith("/")) {
url = url.slice(0, -1);
}
if (url.endsWith("/web")) {
url = url.slice(0, -4);
url += "/rpc";
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

We don't need another config key for /rpc endpoint. the endpoint config key is made for this. It's supposed to be the url where to fetch the data. You just have to write in the documentation that endpoint should contain the rpc url.

}
}
const response = await fetch(url, options);
Copy link
Owner

Choose a reason for hiding this comment

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

use this.fetch here.

// If the session id is not set, we need to get it from the response
if (response.status === 409 && !this.sessionId) {
const body = await response.text();
const match = body.match(/X-Transmission-Session-Id: ([a-z0-9]+)/i);
Copy link
Owner

Choose a reason for hiding this comment

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

I think the X-Transmission-Session-Id exist in the response headers, it would be easier / cleaner to get it from the headers.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can use response.headers.get('x-transmission-session-id') to get the session-id

@SenorSmartyPants
Copy link
Contributor

I'm testing this PR, but I'm stuck on enabling CORS. I have transmission RPC enabled (works with android remote apps), but I don't see a way to enable CORS natively in transmission. Are you putting it behind a proxy to enable this?

@SenorSmartyPants
Copy link
Contributor

I got CORS working on NGINX for this by following this and then modifying the Access-Control-Allow-Headers in the OPTIONS section to add X-Transmission-Session-Id

add_header 'Access-Control-Allow-Headers' 'X-Transmission-Session-Id,DNT,User-Agent,X-Requested-With,If-Modified-Since,Cache-Control,Content-Type,Range';

I need to do that to get my version 3.x transmission to work. But on my other transmission install that was already 4.x I didn't need CORS at all and I could communicate without a proxy. I ended up upgrading my 3.x install to 4.x

Thank you for this PR.

@SenorSmartyPants
Copy link
Contributor

I added some changes to this as a PR into jag-k repo

jag-k#2

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.

Custom Service for Transmission
5 participants