-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[ Demo ] @uppy/companion: express 5.0.0 compatibility #5457
base: main
Are you sure you want to change the base?
Conversation
Diff output filesdiff --git a/packages/@uppy/companion/lib/companion.js b/packages/@uppy/companion/lib/companion.js
index 9416e5e..25f3982 100644
--- a/packages/@uppy/companion/lib/companion.js
+++ b/packages/@uppy/companion/lib/companion.js
@@ -88,7 +88,7 @@ module.exports.app = (optionsArg = {}) => {
// Making `POST` request to the `/connect/:provider/:override?` route requires a form body parser middleware:
// See https://github.com/simov/grant#dynamic-http
app.use(
- "/connect/:oauthProvider/:override?",
+ "/connect/:oauthProvider/:override",
express.urlencoded({ extended: false }),
getCredentialsOverrideMiddleware(providers, options),
);
@@ -102,7 +102,7 @@ module.exports.app = (optionsArg = {}) => {
});
app.use(middlewares.cors(options));
// add uppy options to the request object so it can be accessed by subsequent handlers.
- app.use("*", middlewares.getCompanionMiddleware(options));
+ app.use(/(.*)/, middlewares.getCompanionMiddleware(options));
app.use("/s3", s3(options.s3));
if (options.enableUrlEndpoint) {
app.use("/url", url());
@@ -171,7 +171,7 @@ module.exports.app = (optionsArg = {}) => {
middlewares.hasSimpleAuthProvider,
controllers.simpleAuth,
);
- app.get("/:providerName/list/:id?", middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.list);
+ app.get("/:providerName/list/:id", middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.list);
// backwards compat:
app.get("/search/:providerName/list", middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.list);
app.post( |
Hi, are you sure that there are no breaking changes for people integrating Companion into their own Express 4.x or people making custom providers? If that would break some cases, this can't be merged as we won't do a another major for a while. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be a breaking change and so needs a new major release of Companion
@@ -103,7 +103,7 @@ module.exports.app = (optionsArg = {}) => { | |||
// override provider credentials at request time | |||
// Making `POST` request to the `/connect/:provider/:override?` route requires a form body parser middleware: | |||
// See https://github.com/simov/grant#dynamic-http | |||
app.use('/connect/:oauthProvider/:override?', express.urlencoded({ extended: false }), getCredentialsOverrideMiddleware(providers, options)) | |||
app.use('/connect/:oauthProvider/:override', express.urlencoded({ extended: false }), getCredentialsOverrideMiddleware(providers, options)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking at https://expressjs.com/en/guide/migrating-5.html#path-syntax i think wee need this instead?
app.use('/connect/:oauthProvider/:override', express.urlencoded({ extended: false }), getCredentialsOverrideMiddleware(providers, options)) | |
app.use('/connect/:oauthProvider{/:override}', express.urlencoded({ extended: false }), getCredentialsOverrideMiddleware(providers, options)) |
source: https://www.npmjs.com/package/path-to-regexp#optional
@@ -117,7 +117,7 @@ module.exports.app = (optionsArg = {}) => { | |||
app.use(middlewares.cors(options)) | |||
|
|||
// add uppy options to the request object so it can be accessed by subsequent handlers. | |||
app.use('*', middlewares.getCompanionMiddleware(options)) | |||
app.use(/(.*)/, middlewares.getCompanionMiddleware(options)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this should be
app.use(/(.*)/, middlewares.getCompanionMiddleware(options)) | |
app.use(middlewares.getCompanionMiddleware(options)) |
@@ -132,7 +132,7 @@ module.exports.app = (optionsArg = {}) => { | |||
|
|||
app.post('/:providerName/simple-auth', express.json(), middlewares.hasSessionAndProvider, middlewares.hasBody, middlewares.hasSimpleAuthProvider, controllers.simpleAuth) | |||
|
|||
app.get('/:providerName/list/:id?', middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.list) | |||
app.get('/:providerName/list/:id', middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.list) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
app.get('/:providerName/list/:id', middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.list) | |
app.get('/:providerName/list{/:id}', middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.list) |
No description provided.