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

Fix postgraphile websocket/subscription omission path #31

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jonathanpeterwu
Copy link

Description

On build for the lambda example webpack bundling fails on omission of subscriptions.js as the npm install path has changed

  • My code matches the project's code style and yarn lint:fix passes.

@benjie
Copy link
Member

benjie commented Sep 19, 2021

Please also update the yarn lock (assuming an upgrade is required to trigger the issue?)

@jonathanpeterwu
Copy link
Author

Updated with package upgrade

Comment on lines 32 to 35
new webpack.NormalModuleReplacementPlugin(
/postgraphile\/build\/postgraphile\/http\/subscriptions\.js$/,
/postgraphile\/build-turbo\/postgraphile\/http\/subscriptions\.js$/,
`${__dirname}/src/postgraphile-http-subscriptions.js`
),
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is actually a fix unless you've set GRAPHILE_TURBO which I don't think is set by default.

Instead, please throw away the yarn.lock changes and duplicate this above block - once for non-turbo and once for turbo.

Thanks!

@SnowballAntrobus
Copy link

SnowballAntrobus commented Sep 23, 2021

@benjie is there a reason in particular we would not want to update to the most recent version of postgraphile?

@benjie
Copy link
Member

benjie commented Sep 23, 2021

No; just that I've tested this on older versions and I don't fancy testing it again because working with Lambda is a PITA. I'd rather go with a "known good" version.

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