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

Update to Hapi 17 #687

Merged
merged 6 commits into from
Dec 8, 2017
Merged

Conversation

ItalyPaleAle
Copy link

Note that TypeScript typings for Hapi 17 are not available yet (see #636), so the types packages have been temporarily removed for Hapi and Boom.

Tests are passing, and linter is happy too.

This should close #636 and #685

Note: TS typings for Hapi 17 are not available yet, so the types packages have been temporarily removed
@apollo-cla
Copy link

@EgoAleSum: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@ItalyPaleAle
Copy link
Author

Regarding the travis CI errors... For Node 4 and 6, it is expected that CI fails, because Hapi 17 uses async/await and requires Node 8+.

For Node 8+: on my machine, it works 3 times out of 4; the other times, I get weird behavior like in the CI case above. I am not sure what's causing it, but it's likely an issue with the tests themselves as they hang on something async. I tried looking into it, but not sure how to fix them.

Alessandro Segala added 3 commits December 6, 2017 17:29
This might make tests more predictable and make them fail less randomly
@orblazer
Copy link

orblazer commented Dec 8, 2017

Hello, your exemple for register plugin on hapi is broken.

This exemple is right :

await server.register(graphqlHapi, {
  path: '/graphql',
  options: {
    graphqlOptions: {
      schema: myGraphQLSchema,
    },
    route: {
      cors: true
    }
  }
});

Thanks for your contribution.

@ItalyPaleAle
Copy link
Author

@orblazer Uhm, are you sure about that? There's no such thing as path in the options arguments for Hapi 17: https://hapijs.com/api#plugins

@ItalyPaleAle
Copy link
Author

Ok, I finally figured it out! The issues were with the Adonis test unit. It took many hours of literally going through the Adonis' source code, but some methods that were supposed to be async were called synchronously.

After I made the tests async (to support Hapi, which is all async), issues with Adonis came out. I'm not sure why it was fine before, but essentially there were situations where the Adonis server was not started or closed properly. It should be all good now.

I've also made sure that the Hapi package is tested only on Node 8.9+ (minimum version for Hapi 17).

All tests pass now, yay!

@martijnwalraven martijnwalraven merged commit 037c13e into apollographql:master Dec 8, 2017
@martijnwalraven
Copy link
Contributor

Thanks a lot for working on this!

@orblazer
Copy link

orblazer commented Dec 8, 2017

@EgoAleSum Ops i have paste a wrong exemple.

I have create a issue here : #689

@ItalyPaleAle
Copy link
Author

@martijnwalraven thanks for merging this, do you have any timeline for the next release? (note the change is not backwards-compatible as you can imagine :) )

@martijnwalraven
Copy link
Contributor

@EgoAleSum Yes, we should probably release these changes as 1.3.0. I was hoping we could get #679 in as well, but if that takes too long we should release without it.

@martijnwalraven
Copy link
Contributor

Published as 1.3.0, thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hapi v17
4 participants