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

Prepare for v2 support #151

Merged
merged 6 commits into from
Jul 11, 2021
Merged

Conversation

rauno56
Copy link
Collaborator

@rauno56 rauno56 commented Jul 8, 2021

The oldest current version supported is from 8 months ago. I'd like to support at least v2(2.0.0 released in 2017, latest 2.x in 2021). However, the initialization is a bit different for that. This PR prepares a few of the things without actually adding support for v2 or changing any of the behaviour to make the next PR more concise and easier to follow.

Check the commits for details, but:

  • supports initialization in the way v2 does it - version checking is a bit hacky TBH, I'm open to a better way if you have any preferences;
  • catches async errors in tests(stumbled upon those implementing v2 support);
  • renames events "ping" and "pong" in tests since those are internally used for heartbeats in v2.

Look out for any linting/formatting inconsistencies, because I didn't find a way to automatically apply those(I see GHA job for prettier but running default prettier gave me tons of markdown changes as well - just isolate to ts files?).

@rauno56 rauno56 requested a review from a team as a code owner July 8, 2021 08:19
@blumamir
Copy link
Contributor

blumamir commented Jul 8, 2021

Thanks @rauno56 ,
@mottibec wrote this instrumentation, can you please take a look?

Regarding the prettier, I usually run yarn prettier from the root of the monorepo and it works.
Can you share how exactly you run prettier in your setup?

@rauno56
Copy link
Collaborator Author

rauno56 commented Jul 8, 2021

Problem solved with linting! Thanks, @blumamir!

@blumamir
Copy link
Contributor

blumamir commented Jul 8, 2021

Problem solved with linting! Thanks, @blumamir!

Great :)
we used to have husky hook running the prettier automatically on git push, but unfortunately husky doesn't work well with monorepos:
typicode/husky#677

I see now in the issue that it might be fixed in version 6 of husky, which we can test and verify one day

@blumamir
Copy link
Contributor

blumamir commented Jul 8, 2021

Problem solved with linting! Thanks, @blumamir!

Great :)
we used to have husky hook running the prettier automatically on git push, but unfortunately husky doesn't work well with monorepos:
typicode/husky#677

I see now in the issue that it might be fixed in version 6 of husky, which we can test and verify one day

opened an issue for that: #152

};

export const createServerInstance = (server?: http.Server) => {
if (version && /^2\./.test(version)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

as current support is only for version 2, we are expecting this if to always be truthy, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Current support is for v3 and v4, so the opposite - we currently expect it to be always falsy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right :)

@blumamir
Copy link
Contributor

blumamir commented Jul 9, 2021

LGTM
@mottibec - do you want to take a look as well?

@rauno56 rauno56 mentioned this pull request Jul 9, 2021
@mottibec
Copy link
Contributor

LGTM
Thanks @rauno56

@blumamir blumamir merged commit 21da6e4 into aspecto-io:master Jul 11, 2021
@rauno56 rauno56 deleted the feat/prep-support-v2 branch July 12, 2021 07:25
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.

3 participants