Update dependencies to allow build for all packages to pass #566
Conversation
- Upgrade Webpack related dependencies because they're conflicting with the projects in `packages` - Run `npm install` using Node 20 to update the `package-lock.json` format
- Make `readFile` synchronous since this part seems to be flaky when running tests - Update CSS plugin variable name to match new dependency name
- Use Node 20 instead of Node 12 - Run `npm ci` instead of `npm install` to prevent unexpected dependency changes from being installed
| const mPath = dir ? path.join(dir, "manifest.json") : "manifest.json"; | ||
| return String(await fs.promises.readFile(mPath, "utf-8")); | ||
| return String(fs.readFileSync(mPath, "utf-8")); |
There was a problem hiding this comment.
I'm curious about the reasons why we need to change this from sync to async.
There was a problem hiding this comment.
good question! i was running tests via npm test locally and noticed that the quip-cli was flaky - particularly around the parts where it would read in pre-created files to compare with the command outputs. my theory is that the test times out before it can completely (and consistently) read the files. by forcing it to be synchronous the flakiness disappeared. i ran it several times consecutively and saw 0 failures
There was a problem hiding this comment.
Makes sense! Thanks for the fix!
| @@ -7,13 +7,15 @@ | |||
| "author": "Quip", | |||
| "main": "dist/handleRichTextBoxKeyEventNavigation.js", | |||
| "scripts": { | |||
| "build": "cross-env-shell NODE_ENV=production webpack --output-library-target umd" | |||
| "build": "cross-env-shell NODE_ENV=production webpack" | |||
There was a problem hiding this comment.
Why do we want to remove --output-library-target umd?
There was a problem hiding this comment.
so i added a newer version of webpack via the webpack-cli dependency. based on my research it seems that the --output-library-target argument has been deprecated. we already configure the output to be umd in our webpack.config.js files anyways so i didn't think it'd be an issue to remove the argument. local builds show that the files are indeed in the correct umd format and tests all pass so that gave me more confidence that the change should be safe
There was a problem hiding this comment.
Got it, thanks for the additional info!
Summary / Background
There were issues when attempting to publish the
nextbranch. These were mostly due to outdated dependencies that conflicted with a recent change to get ourChevronandhandleRichTextBoxKeyEventNavigationpackages to use an updated version ofquip-apps-webpack-config.