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

WIP Refactor (breaking?): replace Express with Fastify and other #559

Closed
wants to merge 24 commits into from

Conversation

ozonep
Copy link

@ozonep ozonep commented Feb 4, 2021

Question Answer
Bug fix
Breaking change
Deprecations

Closes #528

Further information:
Tried to "modernize" back-end part to use actively maintained & faster alternatives for some frameworks/libraries.
The biggest change - move from almost dead "Express.js" to modern alternative "Fastify". Replaced all Express plugins with Fastify alternatives (except one - for zip creation, see below)

NOTES: Need some help with these issues:

  • Can't find Fastify plugin for creating zip files.
  • Not sure if "app.set('json replacer', (k, v) => (v === null ? undefined : v))" is still needed in the codebase, after all the changes.

@NivLipetz
Copy link
Member

NivLipetz commented Feb 9, 2021

Hi @ozonep how are you?
Thanks for the interest in closing #528 and for the great effort!

I checked out to your branch and it seems that there is much more work to do, so I was wondering if you were aware of it/are running predator locally while working on the feature to see this? Also you can run the tests locally to see the passing/failing ones.

As I said from my perspective I feel like the work left to do on this MR is substantial and just wanted to know if you had a specific motive of upgrading express to fastify or just for the overall improvement of predator?

Thanks again!

@ozonep
Copy link
Author

ozonep commented Feb 9, 2021

Hi @ozonep how are you?
Thanks for the interest in closing #528 and for the great effort!

I checked out to your branch and it seems that there is much more work to do, so I was wondering if you were aware of it/are running predator locally while working on the feature to see this? Also you can run the tests locally to see the passing/failing ones.

As I said from my perspective I feel like the work left to do on this MR is substantial and just wanted to know if you had a specific motive of upgrading express to fastify or just for the overall improvement of predator?

Thanks again!

Hello! )
I liked this tool & I believe it should gain more attraction and contributors!
it makes more sense to use Fastify/"got" to future proof Predator & reduce number of critical vulnerabilities.
I am open for negative feedback :-D
Right now I am working on updating tests.

I guess this should be released as v2.0.0 because of all the changes...
Anyway, I can keep this as just my fork of the project for some time.

@ozonep
Copy link
Author

ozonep commented Feb 9, 2021

@NivLipetz any ideas why test "call kafka produce with StreamingMessage when streaming manager initialized as kafka manager" fails sometimes because of date not being equal?
Example:
expected: "published_at":"2021-02-09T11:04:30.359Z"
actual: "published_at":"2021-02-09T11:04:30.360Z"

This happens ~1 time out of 10...

@NivLipetz
Copy link
Member

NivLipetz commented Feb 9, 2021

@NivLipetz any ideas why test "call kafka produce with StreamingMessage when streaming manager initialized as kafka manager" fails sometimes because of date not being equal?
Example:
expected: "published_at":"2021-02-09T11:04:30.359Z"
actual: "published_at":"2021-02-09T11:04:30.360Z"

This happens ~1 time out of 10...

yea it's an unstable test, i stubbed the prototype of Date constructor in order for it to always equal in the expectation but i guess it didn't work as i expected it to, I would ignore that failure in the unit-tests.

You can also fix the assertion to check a delta in the timerange where a 10ms difference can be "acceptable" and pass in order to make the test more stable. Here i see the idfference is 1ms

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.

Migrate from request to an alternative
2 participants