DeepLcom / deepl-node

Official Node.js library for the DeepL language translation API.
MIT License
370 stars 22 forks source link

Translator Option 'serverUrl' is misleading #46

Open jblossey opened 6 months ago

jblossey commented 6 months ago

Describe the bug

According to RFC1738 a HTTP URL takes the form of http://<host>:<port>/<path>?<searchpart>.

When setting the serverUrl in the TranslatorOptions, it only replaces the host:port part.

To Reproduce

Construct a new Translator:

const translator = new deepl.Translator(
      process.env.DEEPL_API_KEY,
      {
          serverUrl: 'http://some-other-host.com/resource',
        },
    );

Translate something through this translator and the server at http://some-other-host.com will receive POST /resource/v2/translate HTTP/1.1.

Expected behavior

The option should replace the whole URL and not add its own path to the request. Alternatively, it should be renamed to use the proper terminology, e.g. replaceHostAndPort.

JanEbbing commented 6 months ago

Hi, thanks for the report.

When setting the serverUrl in the TranslatorOptions, it only replaces the host:port part.

This is incorrect, as your example shows (/resource is included in the final URL). We just also append the correct endpoint, as our API has separate endpoints for separate functionality (text translation, document translation, querying usage, etc). So, what you ask for (being able to set the final server URL) is not possible, you need to point it at a URL where the API endpoints are hosted. This can then be used with our mock server, for example.

I agree we can find a clearer name like setApiBase or similar, but I'm not sure it's worth making a breaking change for this (and having both seems unnecessary and confusing).