Skip to content

Conversation

@chemitaxis
Copy link
Contributor

This PR solves this issue proposal: #531

:)

@chemitaxis
Copy link
Contributor Author

I don't know why is failing the CI 😢 I thinks a problem installing a package:

Command failed: npm install @tanstack/svelte-query

@ymc9 ymc9 changed the base branch from main to dev July 2, 2023 02:14
@ymc9
Copy link
Member

ymc9 commented Jul 2, 2023

I don't know why is failing the CI 😢 I thinks a problem installing a package:

Command failed: npm install @tanstack/svelte-query

I believe it's due to the recent release of svelte V4. Please use "dev" branch as the source branch for making changes in the future, and it should work then :D.

logger: options.logger,
});
response.status(r.status).json(marshalToObject(r.body, useSuperJson));
if (manageCustomResponse) {
Copy link
Member

Choose a reason for hiding this comment

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

Just to make sure I understand how to use this. Since when manageCustomResponse is on the code here, simply returns the status and body, is it supposed to use the middleware like regular function call in a request handler then?

const zenstack = ZenStackMiddleware({ manageCustomResponse: true, ... });

app.all('/api/model/*', async (req, res) => {
    const { status, body } = await zenstack(req, res);
    // more processing
    ...
    return res.status(status).json(body);
});

@chemitaxis
Copy link
Contributor Author

Hi, yes @ymc9, this is the idea but I don't know why this is not working... I have pushed a new temporal test before merging with the other express tests with manageCustomResponse: true but it's not working as expected... 🤔 the test never finish.

You can run only this test using this command from server folder

./node_modules/.bin/jest tests/adapter/express-temp.test.ts --silent=false

The silent arg is for checking console.log debugging.

I am trying to figure out where is the problem...

@ymc9
Copy link
Member

ymc9 commented Jul 3, 2023

Hey @chemitaxis

I think the problem is that when manageCustomResponse is set true, the middleware neither sends a response nor calls next(), so the processing is stuck.

I feel the general pattern of express middleware, if not sending a response, is not to return some data - I'm not sure how the returned data would be used by subsequent middleware or route handler. Instead, should it attach information to req or res object and call next(), and subsequent handlers fetch data there and decide what to do? res.locals looks like a good place to store custom data?

@chemitaxis
Copy link
Contributor Author

Hi @ymc9 yes, exactly, there was the problem... I found it but I don't know why, I don't have notifications on my inbox when I get mentioned... I have created a test, what do you think? Best.

@chemitaxis chemitaxis requested a review from ymc9 July 3, 2023 11:27
@rsaladocid
Copy link

this will be very useful @chemitaxis !

@ymc9
Copy link
Member

ymc9 commented Jul 3, 2023

It looks great now! I'll merge it and publish a new version soon. Thank you!

@ymc9 ymc9 merged commit 579b818 into zenstackhq:dev Jul 3, 2023
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.

4 participants