Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix dev server unhandled error #2420

Merged
merged 5 commits into from
May 28, 2020
Merged

Fix dev server unhandled error #2420

merged 5 commits into from
May 28, 2020

Conversation

jimbo
Copy link
Contributor

@jimbo jimbo commented May 22, 2020

Description

Fix an unhandled error wherein the dev server attempts to read tap into the webpack compilation to read GraphQL query files. It looks like the shape of an argument changed somewhere.

Related Issue

Maybe related to PWA-169.

Acceptance

Verification Stakeholders

Specification

Verification Steps

  1. yarn watch:venia
  2. Verify that the node process does not warn about an unhandled error.
  3. Verify that the app is successfully built and served.

Screenshots / Screen Captures (if appropriate)

Checklist

  • I have updated the documentation accordingly, if necessary.
    [ ] I have added tests to cover my changes, if necessary.

Fix an unhandled error wherein the dev server attempts to read
tap into the webpack compilation to read GraphQL query files.
It looks like the shape of an argument changed somewhere.
@jimbo jimbo added the version: Patch This changeset includes backwards compatible bug fixes. label May 22, 2020
/**
* Stats in an array because we have 2 webpack child
* compilations, 1 for client and other for service worker.
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this still true? The argument to this function certainly wasn't an array; perhaps this is still the case in production, though?

Also, the argument to this function appears to be Stats {} itself. I don't see how we could have ever been destructuring stats from the argument in the first place.

Copy link
Contributor

@revanth0212 revanth0212 May 22, 2020

Choose a reason for hiding this comment

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

Nice catch @jimbo. We changed the webpack config to only have 1 compilation during the build process. It is weird that, we never had tests around this. Can we add a test to replicate this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Funny, the tests failed now. May be we were mocking stuff and using that to test the plugin. Hence it never failed when I changed the webpack.config.js in #2390.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented May 22, 2020

Messages
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖

Associated JIRA tickets: PWA-169.

Generated by 🚫 dangerJS against a1b2df6

@devops-pwa-codebuild
Copy link
Collaborator

devops-pwa-codebuild commented May 22, 2020

Performance Test Results

The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate.

https://pr-2420.pwa-venia.com : LH Performance Expected 0.85 Actual 0.57, LH Best Practices Expected 1 Actual 0.92
https://pr-2420.pwa-venia.com/venia-tops.html : LH Performance Expected 0.75 Actual 0.48, LH Best Practices Expected 1 Actual 0.92
https://pr-2420.pwa-venia.com/valeria-two-layer-tank.html : LH Performance Expected 0.8 Actual 0.51, LH Accessibility Expected 0.9 Actual 0.89, LH Best Practices Expected 1 Actual 0.92

revanth0212
revanth0212 previously approved these changes May 22, 2020
Copy link
Contributor

@revanth0212 revanth0212 left a comment

Choose a reason for hiding this comment

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

No errors while running yarn watch:venia

image

Copy link
Contributor

@sirugh sirugh left a comment

Choose a reason for hiding this comment

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

This fixes a problem with graphiql. We need this urgently!

@dpatil-magento dpatil-magento merged commit 2f7861f into develop May 28, 2020
@dpatil-magento dpatil-magento deleted the jimbo/webpack-fix branch May 28, 2020 22:51
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
pkg:pwa-buildpack version: Patch This changeset includes backwards compatible bug fixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants