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(deps): Move build dependencies to non-dev dependencies #1107

Merged
merged 10 commits into from
Feb 24, 2023
Merged

Conversation

queengooborg
Copy link
Collaborator

@queengooborg queengooborg commented Feb 20, 2023

This PR moves dependencies required for building from the devDependencies to regular dependencies.

@caugner
Copy link
Contributor

caugner commented Feb 20, 2023

@queengooborg This timeout is weird. Were you able to run the tests locally?

> @mdn/bob@3.0.0 jest
> JEST_PUPPETEER_CONFIG=jest-puppeteer.config.cjs NODE_OPTIONS=--experimental-vm-modules jest

Server has taken more than 5000ms to start.


☝️ You can set "server.launchTimeout" in jest-puppeteer.config.js

@queengooborg
Copy link
Collaborator Author

queengooborg commented Feb 20, 2023

I was not, and it's actually failing on the main branch as well; you can see the fails in all the open Dependabot PRs. This seems to be caused by some incompatibility with NodeJS v18 (tests pass in 8df94dd using NodeJS v16).

@queengooborg queengooborg changed the title Move "mini-css-extract-plugin" to non-dev dependencies fix(deps): Move "mini-css-extract-plugin" to non-dev dependencies Feb 20, 2023
@queengooborg queengooborg changed the title fix(deps): Move "mini-css-extract-plugin" to non-dev dependencies fix(deps): Move build dependencies to non-dev dependencies Feb 21, 2023
@caugner
Copy link
Contributor

caugner commented Feb 22, 2023

@queengooborg Did you check (and how) if those two dependencies are all that need to be moved?

@queengooborg
Copy link
Collaborator Author

queengooborg commented Feb 22, 2023

I checked by running the following:

cd mdn-interactive-examples
npx install-local ../mdn-bob
npm run build

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@caugner

This comment was marked as outdated.

@caugner
Copy link
Contributor

caugner commented Feb 22, 2023

Did I miss anything?

It's npx install-local.

But I'm still getting an error 😕:

% npm run build                                                                                                                     ✭

> interactive-examples@1.0.4 build
> mdn-bob

node:internal/modules/cjs/loader:995
  const err = new Error(message);
              ^

Error: Cannot find module 'path-browserify'
Require stack:
- /Users/claas/github/mdn/interactive-examples/node_modules/@mdn/bob/webpack.config.js
    at Module._resolveFilename (node:internal/modules/cjs/loader:995:15)
    at Function.resolve (node:internal/modules/cjs/helpers:109:19)
    at file:///Users/claas/github/mdn/interactive-examples/node_modules/@mdn/bob/webpack.config.js:72:21
    at ModuleJob.run (node:internal/modules/esm/module_job:193:25)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:530:24)
    at async loadESM (node:internal/process/esm_loader:91:5)
    at async handleMainPromise (node:internal/modules/run_main:65:12) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/Users/claas/github/mdn/interactive-examples/node_modules/@mdn/bob/webpack.config.js'
  ]
}

Node.js v18.12.1

@queengooborg
Copy link
Collaborator Author

It's npx install-local.

Oops, wrote that reply from my phone and forgot the command for a second... 😆

But I'm still getting an error 😕:

That's odd, I wonder why that error didn't pop up on my end...

@queengooborg
Copy link
Collaborator Author

Ah, I realize now what I was doing wrong; I was running npm i after npx install-local. 🤦

Anyways, I've also updated the test to also perform a clean install before building, and then install the dev dependencies before running the tests with Jest. It looks like that was the last dependency that shouldn't have been a dev dependency!

@caugner
Copy link
Contributor

caugner commented Feb 23, 2023

@queengooborg Thank you. I have got another error now though:

image

Here's what I did:

cd bob
npm run build
cd ../interactive-examples
npx install-local
npm run build

@queengooborg
Copy link
Collaborator Author

😠

It looks like this has something to do with folder resolution with Webpack -- I'll submit another PR to fix that up to keep the scope of this PR down!

@queengooborg
Copy link
Collaborator Author

This is now all that's left to get BoB fixed in production!

@caugner
Copy link
Contributor

caugner commented Feb 24, 2023

🍿

@caugner caugner merged commit 0b03a8f into main Feb 24, 2023
@caugner caugner deleted the devdeps branch February 24, 2023 12:53
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.

2 participants