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 webpack-dev-server middleware application logic #5149

Closed
wants to merge 6 commits into from

Conversation

Swapnilden
Copy link

@Swapnilden Swapnilden commented Apr 11, 2024

This commit refactors the middleware application logic in the webpack-dev-server codebase to ensure that middleware is applied only once, regardless of how many times it's called.

Previously, certain middleware, such as static serving middleware, was being applied multiple times to support features like historyApiFallback. This refactor eliminates duplicate middleware application by introducing a mechanism to track applied middleware and applying each middleware only once.

Changes:

  • Introduced appliedMiddleware array to track applied middleware.
  • Created applyMiddlewareOnce function to apply middleware only if it hasn't been applied before.
  • Updated webpack-dev-server codebase to utilize applyMiddlewareOnce for applying middleware associated with various features.

This update aims to improve code efficiency and prevent unintended behavior or performance issues caused by duplicate middleware application.

Fixes: #2716

  • 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?

Motivation / Use-Case

Breaking Changes

Additional Info

This commit refactors the middleware application logic in the webpack-dev-server codebase to ensure that middleware is applied only once, regardless of how many times it's called. 

Previously, certain middleware, such as static serving middleware, was being applied multiple times to support features like `historyApiFallback`. This refactor eliminates duplicate middleware application by introducing a mechanism to track applied middleware and applying each middleware only once.

Changes:
- Introduced `appliedMiddleware` array to track applied middleware.
- Created `applyMiddlewareOnce` function to apply middleware only if it hasn't been applied before.
- Updated webpack-dev-server codebase to utilize `applyMiddlewareOnce` for applying middleware associated with various features.

This update aims to improve code efficiency and prevent unintended behavior or performance issues caused by duplicate middleware application.

Fixes: webpack#2716
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Please add test cases

lib/Server.js Outdated
applyMiddlewareOnce.call(this, proxyMiddleware);
}

if (this.options.contentBase !== false) {
Copy link
Member

Choose a reason for hiding this comment

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

We don't have contentBase anymore

In this updated version, I have included test cases to ensure that the applyMiddlewareOnce function behaves as expected. Additionally, I have removed references to contentBase, as requested.
Copy link
Author

@Swapnilden Swapnilden left a comment

Choose a reason for hiding this comment

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

I have included test cases in new commit to ensure that the applyMiddlewareOnce function behaves as expected. Additionally, I have removed references to contentBase, as requested.

lib/Server.js Outdated
expect(app.use).toHaveBeenCalledWith(middlewareFunction1);
expect(app.use).toHaveBeenCalledWith(middlewareFunction2);
});
});
Copy link
Member

Choose a reason for hiding this comment

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

We should not store it here

Copy link
Author

Choose a reason for hiding this comment

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

So please suggest for proceedings

Copy link
Member

Choose a reason for hiding this comment

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

We have the test directory

Copy link
Author

Choose a reason for hiding this comment

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

can you suggest me file name , it can be big help

Copy link
Author

Choose a reason for hiding this comment

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

open-option.test.js or proxy-option.test.js

@alexander-akait
Copy link
Member

Closing due to inactivity. Thanks!

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.

behavior of static and historyApiFallback
2 participants