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

feat: use @hapi/hapi in the hapi package #472

Merged
merged 1 commit into from
Oct 3, 2020

Conversation

makepanic
Copy link
Contributor

Thanks for this great library

Summary

This updates the hapi package to use the newer @hapi/hapi types which were introduced in 18.2.0

Description

This only adjusts some typescript imports to make it work when using newer hapi versions.

Note: @hapi/boom is added as dev dependency because @types/hapi__boom says to use @hapi/boom as it ships the correct type definitions:

https://www.npmjs.com/package/@types/hapi__boom

@hapi/boom provides its own type definitions, so you don't need @types/hapi__boom installed!

Technical debt & future

This might be a breaking change as users with old hapi might have incompatible type definitions when trying to register this plugin over the old hapi types.

@changeset-bot
Copy link

changeset-bot bot commented Oct 1, 2020

⚠️ No Changeset found

Latest commit: 4ab3dd1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@makepanic
Copy link
Contributor Author

makepanic commented Oct 1, 2020

Hm seems like this fails to test on node@10 because @hapi/[email protected] requires node >= 12.0:

error @hapi/[email protected]: The engine "node" is incompatible with this module. Expected version ">=12.0.0". Got "10.22.1"

One could work around this by doing a yarn install --ignore-engines but that might hide real compatibility issues 🤔

@tdeekens
Copy link
Owner

tdeekens commented Oct 1, 2020

Many thanks for the contributions!

One could work around this by doing a yarn install --ignore-engines but that might hide real compatibility issues 🤔

Yes I think that would only hide it here. Ultimately, we might then have to change the compatibility as a result and consider this a breaking change. What is your feeling towards that and the usage of node 12 in the Hapi ecosystem?

@makepanic
Copy link
Contributor Author

It seems like node >= 12 was introduced in hapi@19 in January.

I don't really like that they dropped node < 12 with node@10 being a lts version still in maintenance mode until 2021.

What is your feeling towards that and the usage of node 12 in the Hapi ecosystem?

I think having a breaking change for the @promster/hapi module makes sense because most of the @hapi modules are already unsupported in node < 12 (click on modules in https://hapi.dev/module/?sort=name ).

@tdeekens
Copy link
Owner

tdeekens commented Oct 1, 2020

Ok. Agreed. Do you want to make the needed changes for the breaking change on this pr and add the respective changeset?

@makepanic
Copy link
Contributor Author

Sure I'll look into it after work and update the PR.

BREAKING CHANGE: now requires node >= 12 and newer hapi package @hapi/hapi
@makepanic
Copy link
Contributor Author

I'm not sure if I did it correctly.

I've adjusted the PR:

  • packages/hapi/package.json engines now require node >= 12 and npm >= 6.9.0 (it's the npm version that was bundled with the 12.0.0 release https://nodejs.org/en/download/releases/ )
  • add the BREAKING CHANGE: commit footer

If I understand it correctly I don't have to adjust the changelog because changeset version will update it automatically right?

Should the promster readme use @hapi/hapi when talking about the hapi package?

@tdeekens
Copy link
Owner

tdeekens commented Oct 3, 2020

If I understand it correctly I don't have to adjust the changelog because changeset version will update it automatically right?

Yes, thanks for everything. I will merge it into a base (other than master) on promster and take it from there.

@tdeekens tdeekens changed the base branch from master to breaking/node-12 October 3, 2020 12:11
@tdeekens tdeekens merged commit 77e9563 into tdeekens:breaking/node-12 Oct 3, 2020
@tdeekens
Copy link
Owner

tdeekens commented Oct 3, 2020

Will be published as @promster/[email protected]. If you'd prefer to also add something to the readme. Feel free :)

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.

2 participants