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: async pathRewrite #397

Merged
merged 8 commits into from Feb 14, 2020
Merged

feat: async pathRewrite #397

merged 8 commits into from Feb 14, 2020

Conversation

rsethc
Copy link
Contributor

@rsethc rsethc commented Feb 13, 2020

Related issue: #374

@ghost
Copy link

ghost commented Feb 13, 2020

DeepCode's analysis on #478d9b found:

  • 0 critical issues. ⚠️ 0 warnings and 1 minor issue. ✔️ 0 issues were fixed.

💬 This comment has been generated by the DeepCode bot, installed by the owner of the repository. The DeepCode bot protects your repository by detecting and commenting on security vulnerabilities or other critical issues.


☺️ If you want to provide feedback on our bot, here is how to contact us.

@coveralls
Copy link

coveralls commented Feb 13, 2020

Coverage Status

Coverage remained the same at 87.778% when pulling 478d9bb on rsethc:master into 483911a on chimurai:master.

@rsethc rsethc requested a review from chimurai February 13, 2020 21:54
@chimurai
Copy link
Owner

chimurai commented Feb 13, 2020

Nice. Looks good.

The following needs to be in place before this PR can be accepted:

  • Updated documentation README.md
  • unit test(s) for pathRewriter
  • unit test types.spec.ts (testing for the async syntax)

You can have a look at:
Similar PR, but for async router: https://github.com/chimurai/http-proxy-middleware/pull/379/files

Unit tests for path rewriter:
https://github.com/chimurai/http-proxy-middleware/blob/master/test/unit/path-rewriter.spec.ts

Types test:
https://github.com/chimurai/http-proxy-middleware/blob/master/test/types.spec.ts
example of async router: https://github.com/chimurai/http-proxy-middleware/blob/master/test/types.spec.ts#L74

@rsethc
Copy link
Contributor Author

rsethc commented Feb 13, 2020

Alright, I'll take a look at solving these requirements. Thanks

@rsethc
Copy link
Contributor Author

rsethc commented Feb 14, 2020

I think this is ready to have another look. One change I made by accident affected the yarn.lock file, let me know if it's preferable to add a commit that undoes that.

@chimurai
Copy link
Owner

Yes, please undo the changes to yarn.lock

I'll checkout the PR to do some additional testing.

@chimurai chimurai changed the title Allow the path rewriter function to be async. feat: async pathRewrite Feb 14, 2020
@chimurai chimurai merged commit 7c2c31f into chimurai:master Feb 14, 2020
@chimurai
Copy link
Owner

Thanks!

Published a new beta version

You can install it with:
npm install http-proxy-middleware@beta or npm install http-proxy-middleware@0.21.0-beta.3

Let me know if everything still works. I'll publish a final shortly after confirmation.

@rsethc
Copy link
Contributor Author

rsethc commented Feb 14, 2020

Thanks, I'll try out this version and let you know if anything goes wrong.

@rsethc
Copy link
Contributor Author

rsethc commented Feb 14, 2020

The "http-proxy-middleware@0.21.0-beta.3" seems to work as expected in what I'm working on.

@chimurai
Copy link
Owner

published final version: http-proxy-middleware@0.21.0

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