Skip to content

fix(#161): use() accepts array#162

Open
jimisaacs wants to merge 1 commit into
hoangvvo:mainfrom
jimisaacs:jimi/161
Open

fix(#161): use() accepts array#162
jimisaacs wants to merge 1 commit into
hoangvvo:mainfrom
jimisaacs:jimi/161

Conversation

@jimisaacs

Copy link
Copy Markdown

This should allow use() to behave more like express.use

@changeset-bot

changeset-bot Bot commented Oct 29, 2021

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: dfd24aa

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@codecov

codecov Bot commented Oct 29, 2021

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (89196b1) to head (dfd24aa).
⚠️ Report is 47 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #162   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           86        87    +1     
=========================================
+ Hits            86        87    +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@hoangvvo

Copy link
Copy Markdown
Owner

Could you let me know some use cases around this? Is there any libraries that require this pattern?

Otherwise, I think this can simply perform a .map to register the handlers:

["/foo", "/bar"].map((base) => nc.use(base, fn))

@jimisaacs

jimisaacs commented Oct 30, 2021

Copy link
Copy Markdown
Author

The use case is for an internal library written for express to be used with next. I'd like to use next without modifying that library and without using a next custom server approach, and I was looking to this library to achieve that, as I read "drop-in replacement".

@hoangvvo hoangvvo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for your response. I left some comments.

Comment thread src/index.js Outdated
Comment thread src/index.d.ts Outdated
@hoangvvo

Copy link
Copy Markdown
Owner

I would be more than happy to merge this. There is just a few nits!

@jimisaacs jimisaacs force-pushed the jimi/161 branch 3 times, most recently from b42c537 to 2806c87 Compare April 10, 2022 15:51
@jimisaacs

Copy link
Copy Markdown
Author

@hoangvvo Ok sorry for the long long delay, but I've reworked this a bit. Should be much much simpler. Only handling one case here. This one:

app.use([middleware1, middleware2]);

Which a lot of middlewares expect.

@hoangvvo

Copy link
Copy Markdown
Owner

@hoangvvo Ok sorry for the long long delay, but I've reworked this a bit. Should be much much simpler. Only handling one case here. This one:

app.use([middleware1, middleware2]);

Which a lot of middlewares expect.

Thank you! So in this, the array middleware pattern only works if it is the first argument. Is it how it works in express.js too? Would there be a use case for having a base then a middeware array?

const mArr = [m1,m2,m3];
handler.use("/foo", mArr);

@jimisaacs

jimisaacs commented Apr 11, 2022

Copy link
Copy Markdown
Author

@hoangvvo if you look at the express code, they support a lot of crazy things. All sorts of positions and levels of arrays. If you look at their types, it's similar, but not quite as open ended as their JavaScript. As an example, in their JavaScript, they use a recursive while loop lookup, just to find the first string argument.

This is the reason I kept it simple. Because the most common use case in the express world, is the one I'm adding support for. The most common use case for middleware is to be added to the app without a route, though yes, they support arrays everywhere middleware is taken, at all levels.

If you are interested in supporting exactly what express does, I can look into that, but it would most likely be just copying a lot of their code. What are you thinking?

@hoangvvo

hoangvvo commented Apr 11, 2022

Copy link
Copy Markdown
Owner

@hoangvvo if you look at the express code, they support a lot of crazy things. All sorts of positions and levels of arrays. If you look at their types, it's similar, but not quite as open ended as their JavaScript. As an example, in their JavaScript, they use a recursive while loop lookup, just to find the first string argument.

This is the reason I kept it simple. Because the most common use case in the express world, is the one I'm adding support for. The most common use case for middleware is to be added to the app without a route, though yes, they support arrays everywhere middleware is taken, at all levels.

If you are interested in supporting exactly what express does, I can look into that, but it would most likely be just copying a lot of their code. What are you thinking?

I saw your previous implementation support this pattern of having array in multiple places, but this implementation is obviously so much cleaner. I am okay with just supporting only the first argument. However, if we go with the other one, another hacky solution is to use Array.flat to just flatten out the array middlewares and process as expected.

@jimisaacs

jimisaacs commented Apr 11, 2022

Copy link
Copy Markdown
Author

That's what express does, but along with some additional processing. Just want to get to know your thoughts here.

@hoangvvo

Copy link
Copy Markdown
Owner

That's what express does, but along with some additional processing. Just want to get to know your thoughts here.

If we would add support to this, I think it's better to support all positions.

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