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

refactor busboy constructor - add esm #61

Merged
merged 17 commits into from
Dec 4, 2021

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Dec 3, 2021

@kibertoad
I know you assigned esm support, but i took the opportunity to also refactor the whole busboy constructor functionality and write corresponding unit tests.

It makes it also possible to set autoDestroy to true or false, if somebody wants to do that.

test coverage for main.js increases to 100%

Checklist

lib/main.js Show resolved Hide resolved
lib/main.js Show resolved Hide resolved
lib/main.js Outdated Show resolved Hide resolved
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Dec 4, 2021

@kibertoad

Maybe if this helps for the approval:
This PR is actually non-backwards-breaking :). You can put the unit tests to master and you will see all the errors thrown are having the same reasons.

123 passing (254ms)
  5 failing

  1) busboy-constructor
       should throw an Error if no options are provided:

      AssertionError: expected [Function] to throw error including 'Busboy expected an options-Object.' but got 'Cannot read property \'highWaterMark\' of undefined'
      + expected - actual

      -Cannot read property 'highWaterMark' of undefined
      +Busboy expected an options-Object.
      
      at Context.<anonymous> (test/main.spec.js:6:40)
      at processImmediate (internal/timers.js:464:21)

  2) busboy-constructor
       should throw an Error if options does not contain headers:

      AssertionError: expected [Function] to throw error including 'Busboy expected an options-Object with headers-attribute.' but got 'Missing Content-Type'
      + expected - actual

      -Missing Content-Type
      +Busboy expected an options-Object with headers-attribute.
      
      at Context.<anonymous> (test/main.spec.js:9:42)
      at processImmediate (internal/timers.js:464:21)

  3) busboy-constructor
       should throw an Error if content-type is not set:

      AssertionError: expected [Function] to throw error including 'Missing Content-Type-header.' but got 'Missing Content-Type'
      + expected - actual

      -Missing Content-Type
      +Missing Content-Type-header.
      
      at Context.<anonymous> (test/main.spec.js:16:55)
      at processImmediate (internal/timers.js:464:21)

  4) busboy-constructor
       should throw an Error if content-type is unsupported:

      AssertionError: expected [Function] to throw error including 'Unsupported Content-Type.' but got 'Unsupported content type: unsupported'
      + expected - actual

      -Unsupported content type: unsupported
      +Unsupported Content-Type.
      
      at Context.<anonymous> (test/main.spec.js:19:86)
      at processImmediate (internal/timers.js:464:21)

  5) busboy-constructor
       if busboy is called with stream options and autoDestroy:true, autoDestroy should be set to true:

      AssertionError: expected false to equal true
      + expected - actual

      -false
      +true
      
      at Context.<anonymous> (test/main.spec.js:36:61)
      at processImmediate (internal/timers.js:464:21)

@kibertoad
Copy link
Member

yeah, it looks good, just need a bit of focus to review it throughly. also want to apply the infamous triplet pattern for cross cjs/esm support.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Dec 4, 2021

@kibertoad

Do you mean this?

if (typeof define === 'function' && define.amd) {
  define('Busboy', function () {
    return Busboy;
  });
} else if (typeof module !== 'undefined' && module.exports) {
  module.exports = Busboy;
  module.exports.default = Busboy
  module.exports.Busboy = Busboy;
} else {
  global.Busboy = Busboy;
}

@kibertoad
Copy link
Member

we've been doing just the second branch without condition in the past. global doesn't seem applicable to node-only lib, do you find first branch useful?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Dec 4, 2021

I just copied and modified it from another project. But yes you are right, the last one is for browser compatibility.

@kibertoad
Copy link
Member

AMD seems frontend-focused too. So I'd suggest keeping just the second branch without any checks.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Dec 4, 2021

@kibertoad

add infamous ejs, cjs triplet :)

@kibertoad
Copy link
Member

We also need to update TS types and add import tests.
See https://github.com/pinojs/sonic-boom/blob/master/types/index.d.ts and https://github.com/pinojs/sonic-boom/blob/master/types/index.test-d.ts for how this can be done for the infamous triplet.

README.md Show resolved Hide resolved
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Dec 4, 2021

@kibertoad
Seems like it was already OK?

test/types/main.test-d.ts Outdated Show resolved Hide resolved
@kibertoad
Copy link
Member

kibertoad commented Dec 4, 2021

@Uzlopak I'm not so sure. Busboy in TS exports only a type, not a constructor. BusboyConstructor is exported as a default, but not as Busboy as well, so you can't do named import for a constructor itself (not a type).

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Dec 4, 2021

@kibertoad
Ok now?

@Uzlopak Uzlopak merged commit 08c3b20 into fastify:master Dec 4, 2021
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.

2 participants