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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
975e329
Fix dev server unhandled error
jimbo May 22, 2020
d8c2e0c
Fix test
jimbo May 22, 2020
86b7d6a
Merge branch 'develop' into jimbo/webpack-fix
jimbo May 27, 2020
1f4a625
Merge branch 'develop' into jimbo/webpack-fix
sirugh May 28, 2020
a1b2df6
Merge branch 'develop' into jimbo/webpack-fix
dpatil-magento May 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 25 additions & 38 deletions packages/pwa-buildpack/lib/WebpackTools/PWADevServer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,54 +164,41 @@ const PWADevServer = {

if (graphqlPlayground) {
const endpoint = '/graphql';

const oldBefore = webpackDevServerOptions.before;

webpackDevServerOptions.before = (app, server) => {
oldBefore(app, server);
let middleware;

const readQueryFile = async filename => {
const query = await readFile(filename, 'utf8');
const name = path.relative(context, filename);

return { endpoint, name, query };
};

const gatheringQueryTabs = new Promise((resolve, reject) => {
const { compiler } = server.middleware.context;
compiler.hooks.done.tap(
'PWADevServer',
async ({ stats }) => {
/**
* 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 •
edited
Loading

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!

const queryFilePaths = [];
for (const { compilation } of stats) {
for (const filename of compilation.fileDependencies) {
if (filename.endsWith('.graphql')) {
queryFilePaths.push(filename);
}

compiler.hooks.done.tap('PWADevServer', async stats => {
try {
const { compilation } = stats;
const { fileDependencies } = compilation;
const tabs = [];

for (const filename of fileDependencies) {
if (filename.endsWith('.graphql')) {
tabs.push(readQueryFile(filename));
}
}
try {
resolve(
await Promise.all(
queryFilePaths.map(async queryFile => {
const query = await readFile(
queryFile,
'utf8'
);
const name = path.relative(
context,
queryFile
);
return {
endpoint,
name,
query
};
})
)
);
} catch (e) {
reject(e);
}

resolve(await Promise.all(tabs));
} catch (error) {
reject(error);
}
);
});
});

/* istanbul ignore next: dummy next() function not testable */
const noop = () => {};
app.get('/graphiql', async (req, res) => {
Expand Down
44 changes: 18 additions & 26 deletions packages/pwa-buildpack/lib/WebpackTools/__tests__/PWADevServer.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,19 +257,15 @@ test('graphql-playground middleware attached', async () => {
use: jest.fn()
};
const compilerStatsData = {
stats: [
{
compilation: {
fileDependencies: new Set([
'path/to/module.js',
'path/to/query.graphql',
'path/to/otherModule.js',
'path/to/otherQuery.graphql',
'path/to/thirdModule.js'
])
}
}
]
compilation: {
fileDependencies: new Set([
'path/to/module.js',
'path/to/query.graphql',
'path/to/otherModule.js',
'path/to/otherQuery.graphql',
'path/to/thirdModule.js'
])
}
};
const compiler = {
hooks: {
Expand Down Expand Up @@ -351,19 +347,15 @@ test('graphql-playground middleware handles error during project query read', as
use: jest.fn()
};
const compilerStatsData = {
stats: [
{
compilation: {
fileDependencies: new Set([
'path/to/module.js',
'path/to/query.graphql',
'path/to/otherModule.js',
'path/to/otherQuery.graphql',
'path/to/thirdModule.js'
])
}
}
]
compilation: {
fileDependencies: new Set([
'path/to/module.js',
'path/to/query.graphql',
'path/to/otherModule.js',
'path/to/otherQuery.graphql',
'path/to/thirdModule.js'
])
}
};
let readHook;
const compiler = {
Expand Down