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

@uppy/companion: upgrade got to 12 #4353

Closed
wants to merge 4 commits into from
Closed

@uppy/companion: upgrade got to 12 #4353

wants to merge 4 commits into from

Conversation

mifi
Copy link
Contributor

@mifi mifi commented Mar 13, 2023

fixes #4317

Seems like jest doesn't like await import on node.js 14 so I've also removed node14. which should be fine because it's EOL in april, and we're releasing major anyways #4317

also upgrade jest to fix jestjs/jest#13008

todo:

Update: getting some strange unrelated TS build errors now. not sure why:

Error: ../../../node_modules/@types/vfile/index.d.ts(11,31): error TS1479: The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("vfile-message")' call instead.

Other benefits of got 12:

seems to have much more descriptive error messages:

got 11:

> (await import('got')).default.post('http://google.com').catch(console.error)
Promise {
  <pending>,
  [Symbol(async_id_symbol)]: 483,
  [Symbol(trigger_async_id_symbol)]: 479
}
> HTTPError: Response code 405 (Method Not Allowed)
    at Request.<anonymous>
    at processTicksAndRejections (node:internal/process/task_queues:96:5) {
  code: 'ERR_NON_2XX_3XX_RESPONSE',
  timings: {
    start: 1680006472391,
    socket: 1680006472391,
    lookup: 1680006472401,
    connect: 1680006472440,
    secureConnect: undefined,
    upload: 1680006472440,
    response: 1680006472515,
    end: 1680006472516,
    error: undefined,
    abort: undefined,
    phases: {
      wait: 0,
      dns: 10,
      tcp: 39,
      tls: undefined,
      request: 0,
      firstByte: 75,
      download: 1,
      total: 125
    }
  }
}

got 12

> (await import('got')).default.post('http://google.com').catch(console.error)
Promise {
  <pending>,
  [Symbol(async_id_symbol)]: 5444,
  [Symbol(trigger_async_id_symbol)]: 5443
}
> HTTPError: Response code 405 (Method Not Allowed)
    at Request.<anonymous>
    at Object.onceWrapper (node:events:628:26)
    at Request.emit (node:events:525:35)
    at Request.emit (node:domain:552:15)
    at Request._onResponseBase
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async Request._onResponse {
  input: undefined,
  code: 'ERR_NON_2XX_3XX_RESPONSE',
  timings: {
    start: 1680006366014,
    socket: 1680006366015,
    lookup: 1680006366016,
    connect: 1680006366051,
    secureConnect: undefined,
    upload: 1680006366051,
    response: 1680006366128,
    end: 1680006366130,
    error: undefined,
    abort: undefined,
    phases: {
      wait: 1,
      dns: 1,
      tcp: 35,
      tls: undefined,
      request: 0,
      firstByte: 77,
      download: 2,
      total: 116
    }
  },
  options: {
    request: undefined,
    agent: { http: undefined, https: undefined, http2: undefined },
    h2session: undefined,
    decompress: true,
    timeout: {
      connect: undefined,
      lookup: undefined,
      read: undefined,
      request: undefined,
      response: undefined,
      secureConnect: undefined,
      send: undefined,
      socket: undefined
    },
    prefixUrl: '',
    body: undefined,
    form: undefined,
    json: undefined,
    cookieJar: undefined,
    ignoreInvalidCookies: false,
    searchParams: undefined,
    dnsLookup: undefined,
    dnsCache: undefined,
    context: {},
    hooks: {
      init: [],
      beforeRequest: [],
      beforeError: [],
      beforeRedirect: [],
      beforeRetry: [],
      afterResponse: []
    },
    followRedirect: true,
    maxRedirects: 10,
    cache: undefined,
    throwHttpErrors: true,
    username: '',
    password: '',
    http2: false,
    allowGetBody: false,
    headers: {
      'user-agent': 'got (https://github.com/sindresorhus/got)',
      'accept-encoding': 'gzip, deflate, br'
    },
    methodRewriting: false,
    dnsLookupIpVersion: undefined,
    parseJson: [Function: parse],
    stringifyJson: [Function: stringify],
    retry: {
      limit: 2,
      methods: [ 'GET', 'PUT', 'HEAD', 'DELETE', 'OPTIONS', 'TRACE' ],
      statusCodes: [
        408, 413, 429, 500,
        502, 503, 504, 521,
        522, 524
      ],
      errorCodes: [
        'ETIMEDOUT',
        'ECONNRESET',
        'EADDRINUSE',
        'ECONNREFUSED',
        'EPIPE',
        'ENOTFOUND',
        'ENETUNREACH',
        'EAI_AGAIN'
      ],
      maxRetryAfter: undefined,
      calculateDelay: [Function: calculateDelay],
      backoffLimit: Infinity,
      noise: 100
    },
    localAddress: undefined,
    method: 'POST',
    createConnection: undefined,
    cacheOptions: {
      shared: undefined,
      cacheHeuristic: undefined,
      immutableMinTimeToLive: undefined,
      ignoreCargoCult: undefined
    },
    https: {
      alpnProtocols: undefined,
      rejectUnauthorized: undefined,
      checkServerIdentity: undefined,
      certificateAuthority: undefined,
      key: undefined,
      certificate: undefined,
      passphrase: undefined,
      pfx: undefined,
      ciphers: undefined,
      honorCipherOrder: undefined,
      minVersion: undefined,
      maxVersion: undefined,
      signatureAlgorithms: undefined,
      tlsSessionLifetime: undefined,
      dhparam: undefined,
      ecdhCurve: undefined,
      certificateRevocationLists: undefined
    },
    encoding: undefined,
    resolveBodyOnly: false,
    isStream: false,
    responseType: 'text',
    url: URL {
      href: 'http://google.com/',
      origin: 'http://google.com',
      protocol: 'http:',
      username: '',
      password: '',
      host: 'google.com',
      hostname: 'google.com',
      port: '',
      pathname: '/',
      search: '',
      searchParams: URLSearchParams {},
      hash: ''
    },
    pagination: {
      transform: [Function: transform],
      paginate: [Function: paginate],
      filter: [Function: filter],
      shouldContinue: [Function: shouldContinue],
      countLimit: Infinity,
      backoff: 0,
      requestLimit: 10000,
      stackAllItems: false
    },
    setHost: true,
    maxHeaderSize: undefined,
    signal: undefined,
    enableUnixSockets: true
  }
}

@Murderlon
Copy link
Member

Murderlon commented Mar 13, 2023

I don't think we should do this.

  • It's error prone to always double await when using the the client
  • the parenthesis around the await are awkward.
  • AFAIK we can still be on 11.x, we are not vulnerable.
  • This forces us to run jest with experimental VM modules which would be better if prevented

@mifi
Copy link
Contributor Author

mifi commented Mar 13, 2023

  • It's error prone to always double await when using the the client

not sure I agree. typescript warns if you forget to await it.

  • the parenthesis around the await are awkward.

that I can agree, but it's still not too bad

  • AFAIK we can still be on 11.x, we are not vulnerable.

yes, I believe we are vulnerable on 11, see #4317

  • This forces us to run jest with experimental VM modules which would be better if prevented

true, but ESM is the future anyways, right?

@Murderlon
Copy link
Member

Murderlon commented Mar 13, 2023

yes, I believe we are vulnerable on 11, see #4317

npm audit is broken by design. Do we have proof we are actually vulnerable in our usage?

true, but ESM is the future anyways, right?

Yes, but at the moment that's mostly true for the browser, not for server-side Node.js


For me the only reason to do this is if we're actually vulnerable. Otherwise the overhead is simply not worth it.

@mifi
Copy link
Contributor Author

mifi commented Mar 13, 2023

npm audit is broken by design. Do we have proof we are actually vulnerable in our usage?

no I don't 🙈

Yes, but at the moment that's mostly true for the browser, not for server-side Node.js

I disagree. I think esm is the future for node.js too. has a lot of benefits over commonjs

@mifi
Copy link
Contributor Author

mifi commented Mar 13, 2023

it could have been this vulnerability which seems it has been withdrawn: sindresorhus/got#2220

anyways we should probably eventually upgrade our npm dependencies so we don't have to stay on older versions and will be missing out on features and security fixes in the future. and because companion cannot be converted to ESM, this seems to be the way to go.

alternatively we have to move away from got, but that's gonna be a larger rewrite. I can imagine other modules we use or want to use in the future are also ESM, so if we are going to take a generic stance against ESM dependencies in companion, that might not be good for the future of companion, as we will be left on old versions of dependencies and many modules will not be possible to use, limiting innovation.

@arturi
Copy link
Contributor

arturi commented Mar 13, 2023

@aduh95 thoughts? it's a tie in Mikael vs Merlijn 🥊

@mifi
Copy link
Contributor Author

mifi commented Mar 13, 2023

Actually when i think of it… we could port companion to ESM. People can still use esm companion as an express middleware from CJS, they just have to await import companion

@Murderlon
Copy link
Member

anyways we should probably eventually upgrade our npm dependencies so we don't have to stay on older versions and will be missing out on features and security fixes in the future.

This is a good argument. But 11.x still receives security fixes so we should be safe. Once something actually vulnerable happens on a version we're on, and it actually effects us, we can reconsider. But for now, I think this upgrade is too soon and only causes complications.

@Murderlon Murderlon changed the title upgrade got to 12 @uppy/companion: upgrade got to 12 Mar 14, 2023
@dschmidt
Copy link
Contributor

  • This forces us to run jest with experimental VM modules which would be better if prevented

This is not necessarily true - you can also use babel-jest to transpile modules on the fly.

(No strong opinions about the topic otherwise, just thought this might be useful information for the discussion)

@mifi
Copy link
Contributor Author

mifi commented Apr 22, 2024

I believe this can be closed because of #5035

@mifi mifi closed this Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants