search

mhazy / hapi-github-webhooks

A hapi authentication strategy plugin for validating webhook requests from GitHub.
https://github.com/mhazy/hapi-github-webhooks
MIT License
3 stars 1 forks source link

hapi v17 support #5

Closed travi closed 6 years ago

travi commented 6 years ago

not a lot of activity here lately, but i suppose not much is needed for a fairly simple plugin.

i've found it useful on some of my projects and would like to keep using it as i migrate them over hapi v17.

if i were to update the plugin to be compatible with hapi v17 would you accept the PR?

mhazy commented 6 years ago

@travi Thanks again for the PR, 2.0.0 has been pushed

travi commented 6 years ago

thanks for accepting and getting the new version published.

unfortunately, it looks like something went wrong with the build, though. when i update to this version, i get the following error:

Error: Cannot find module './hmac'
           at Function.Module._resolveFilename (module.js:538:15)
           at Function.Module._load (module.js:468:25)
           at Module.require (module.js:587:17)
           at require (internal/module.js:11:18)
           at Object.<anonymous> (/path/to/my/project/node_modules/hapi-github-webhooks/lib/index.js:9:14)
           at Module._compile (module.js:643:30)
           at Module._extensions..js (module.js:654:10)
           ...

(the path that is shown in the output points to the source file under lib/ because of sourcemaps. lib/ missing from the bundle should be unrelated

i did not have this error when depending on the forked version that i published: https://www.npmjs.com/package/@travi/hapi-github-webhooks. it turns out that when i published that version, i had a dirty dist/ that contained index.js and hmac.js. while this makes my published version work, it is unexpected for how i set up the rollup build. since it worked for me, i didnt pay enough attention to realize that the modules were not using es syntax. rollup needs the modules to be es modules, which is why they get left out of dist/ when doing the build as-is.

there are two ways to fix this problem:

i'm happy to send another PR to get this fixed, but wanted to see if you had a preference of which path to follow. thoughts?

mhazy commented 6 years ago

Oops, yeah. Converted things over and things are working again. Noticing an issue with responseToolkit.continue now though when run on 17.2 -- can dig into it later today and push up a PR.

Tossed a deprecation on 2.0.0

image

travi commented 6 years ago

Tossed a deprecation on 2.0.0

Good call.

Sorry i didnt catch it in my initial PR. normally i use rimraf to clean out build directories before generating the new assets, but I didn't go that far with this. Had I done so, I would have noticed this issue.

Noticing an issue with responseToolkit.continue now though when run on 17.2

Let me know if there is anything you need from me to finish things up. I'm happy to at least test again once you have another release ready if you dont need anything before that.

travi commented 6 years ago

any further progress with this? need any help from me to get it over the finish line?

travi commented 6 years ago

what are your thoughts on this? do you want me to take another stab at getting this working? if you're interested in having an additional maintainer on the project, i'd be open to discussing options to help out.

mhazy commented 6 years ago

@travi -- I re-arranged things awhile ago but it fell off my radar, can you take a look at the following branch and see if that'll work out? Would be good to be able to have tests for specific versions of hapi

https://github.com/mhazy/hapi-github-webhooks/tree/feature/es-modules

Feel free to branch that and adjust and submit a PR, definitely open to move this to somewhere to be maintained by more people as needed.

travi commented 6 years ago

sorry for the delayed response. finally getting a chance to take a look again, so hopefully ill have some updates to send your way somewhat soon.