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

feat: add setupMiddlewares option #4068

Merged
merged 9 commits into from Dec 18, 2021

Conversation

snitin315
Copy link
Member

@snitin315 snitin315 commented Dec 2, 2021

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

Yes.

Motivation / Use-Case

setupMiddleware: (arrayOfMiddlewares, devServer) => {

  arrayOfMiddlewares.push('something');

  return arrayOfMiddlewares;
}

TODO:

  • Deprecate onAfterSetupMiddleware and onBeforeSetupMiddleware.
  • E2E tests
  • Add examples in docs

Breaking Changes

None

Additional Info

No

@codecov
Copy link

codecov bot commented Dec 2, 2021

Codecov Report

Merging #4068 (ec1fba2) into master (0ed7d9e) will increase coverage by 0.29%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4068      +/-   ##
==========================================
+ Coverage   92.25%   92.54%   +0.29%     
==========================================
  Files          14       14              
  Lines        1484     1462      -22     
  Branches      548      552       +4     
==========================================
- Hits         1369     1353      -16     
+ Misses        107      101       -6     
  Partials        8        8              
Impacted Files Coverage Δ
lib/Server.js 93.92% <100.00%> (+0.39%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ed7d9e...ec1fba2. Read the comment docs.

@snitin315 snitin315 force-pushed the feat/setup-middlewares-option branch from 6768a2a to 911a5e5 Compare December 3, 2021 09:43
@snitin315 snitin315 marked this pull request as ready for review December 3, 2021 09:44
@alexander-akait
Copy link
Member

Refactored and updated examples + improve perf for setHeaders, we don't need to attach middleware if we don't have headers

@alexander-akait
Copy link
Member

Also refactor some places, so we now try to use connect insta instead express and maybe even other servers, so in future we can migrate on any application

@alexander-akait
Copy link
Member

@snitin315 Ideally we need add new options server: { type: "connect", options: ...options }, type can be "string" (i.e. package name or require.resolve("connect")) or function, and ideally we should tests all our functions on compatibility with connect - I think we can create new CI pipeline and setup tests to using connect instead express using jest aliases

@alexander-akait alexander-akait merged commit c13aa56 into master Dec 18, 2021
@alexander-akait alexander-akait deleted the feat/setup-middlewares-option branch December 18, 2021 14:55
@snitin315
Copy link
Member Author

@snitin315 Ideally we need add new options server: { type: "connect", options: ...options }, type can be "string" (i.e. package name or require.resolve("connect")) or function, and ideally we should tests all our functions on compatibility with connect - I think we can create new CI pipeline and setup tests to using connect instead express using jest aliases

Yes, I think we can create a separate pipeline for connect. I will try adding one.

@alexander-akait
Copy link
Member

@snitin315 I was wrong, we need application: { type: "connect", options: ...options } (default value is express) in long term we should migrate on connect or maybe something more simple, express is not get updated a long time, but we need to support http2/https without any problems and workarounds, I think now we can setup our tests and fix code for connect and them add this options, so we can have two-three PRs

@Airkro
Copy link

Airkro commented Dec 30, 2021

The API is not easy to understand like before, what's the advantage of setupMiddlewares?

@alexander-akait
Copy link
Member

What is misleading? It was implemented to allow developers use own middlewares and even replaces built-in

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.

None yet

3 participants