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

Move framework-tools deps to root package.json #3908

Merged
merged 10 commits into from
Dec 16, 2021
Merged

Conversation

jtoar
Copy link
Contributor

@jtoar jtoar commented Dec 15, 2021

This PR gets rid of the package.json in framework-tools. Its dependencies and scripts are now in the root workspace's package.json.

At first I thought that if we merged this, it would break yarn rwfw. But curiously yarn rwfw still seemed to work and that's because yarn 3 is awesome. For example, right now yarn rwfw project:deps expands to this:

yarn --cwd ~/prjcts/rw/redwood/tasks/framework-tools project:deps

Even though there's no package.json in framework-tools, yarn 3 knows to look in the root workspace's package.json for a script named project:deps.

In addition to the above, this PR also

  • Fixes a few spelling mistakes
  • Changes the node std lib imports in framework-tools to use the node protocol (e.g. import path from 'node:path')

@jtoar
Copy link
Contributor Author

jtoar commented Dec 15, 2021

@dac09 Requested your review since I've broken this a few times and just want you to double check that works.

@@ -1,11 +1,12 @@
#!/usr/bin/env node
/* eslint-env node */

import path from 'path'
import path from 'node:path'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you doing this here? I'm confused... why is this necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the node protocol. It's just a new feature in ES Modules: https://nodejs.org/api/esm.html#node-imports. There's no meaningful difference; the only justification I have is

  1. using URLs is more web-y than the previous way of doing things
  2. it's easier for beginners (they can tell that this is from the node std lib)

The Node.js docs will most likely use it in the future too: nodejs/node#38343.

Can remove it though; just added it cause I liked it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like it. Keep us moving in that direction, k? My only concern would be using features that aren't supported in v16 (i.e. > 16 only).

@@ -170,15 +169,15 @@ export function buildPackages(packages = frameworkPkgJsonFiles()) {
{
shell: true,
stdio: 'inherit',
cwd: path.resolve(__dirname, '../../../../'),
cwd: path.resolve(__dirname, '../../../'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, not sure why this path has changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did move it actually; the filepath used to be tasks/framework-tools/src/lib/framework.mjs.

@dac09
Copy link
Contributor

dac09 commented Dec 15, 2021

At first I thought that if we merged this, it would break yarn rwfw. But curiously yarn rwfw still seemed to work and that's because yarn 3 is awesome.

This is cool! However, should we be updated rwfw to point to the correct path? The fact that its working might be us being lucky, if we're moving this stuff over I'd rather migrate the whole thing properly - because otherwise we might hit bugs that come out of nowhere!

@jtoar
Copy link
Contributor Author

jtoar commented Dec 15, 2021

This is cool! However, should we be updated rwfw to point to the correct path?

I did here:

execa.sync('yarn', ['--cwd', absRwFwPath, ...command], {

@jtoar jtoar requested a review from dac09 December 15, 2021 18:53
Copy link
Contributor

dac09 commented Dec 16, 2021

Nice! I'll take it for a spin in the morning

@dac09
Copy link
Contributor

dac09 commented Dec 16, 2021

@jtoar unfortunately looks like generate test project is broken (just FYI, its also broken since the last framework change we did!)

@jtoar
Copy link
Contributor Author

jtoar commented Dec 16, 2021

Swore I tested it when I moved its dependency on jscodeshift up but maybe I didn't clean dist/node_modules.

@dac09 the fix was just using yarn jscodeshift for all platforms instead of just Windows since the location of the jscodeshift binary is now at the top level.

Copy link
Contributor

dac09 commented Dec 16, 2021

haha yeah it was probably one of those pesky “yarn forgot to remove things” situations. Getting a failure on rwfw in gitpod btw:


[19:04:36] [link] Building Redwood framework [completed]
[19:04:36] [link] Adding framework dependencies to project [started]
Usage Error: Couldn't find a script named "framework".

$ yarn run [--inspect] [--inspect-brk] <scriptName> ...
[19:04:37] [link] Adding framework dependencies to project [failed]
[19:04:37] → Command failed with exit code 1: yarn framework project:deps
Error: Command failed with exit code 1: yarn framework project:deps
    at makeError (/workspace/redwood/node_modules/execa/lib/error.js:60:11)
    at handlePromise (/workspace/redwood/node_modules/execa/index.js:118:26)
    at processTicksAndRejections (node:internal/process/task_queues:96:5) {
  shortMessage: 'Command failed with exit code 1: yarn framework project:deps',
  command: 'yarn framework project:deps',
  escapedCommand: '"yarn framework project:deps"',
  exitCode: 1,
  signal: undefined,
  signalDescription: undefined,

But I suspect…. its because the older version of rwfw points at the framework folder?

Copy link
Contributor Author

jtoar commented Dec 16, 2021

It looks like it was because the execa commands that the --link flag was running still had framework in them (yarn framework project:deps): 25e17bd.

@jtoar
Copy link
Contributor Author

jtoar commented Dec 16, 2021

Rebased to fix merge conflicts.

Copy link
Contributor

@thedavidprice thedavidprice left a comment

Choose a reason for hiding this comment

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

This is gtg for me. Deferring to Danny.

@jtoar jtoar mentioned this pull request Dec 16, 2021
1 task
Copy link
Contributor

@dac09 dac09 left a comment

Choose a reason for hiding this comment

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

BAZINGA!

@dac09 dac09 enabled auto-merge (squash) December 16, 2021 23:13
@dac09 dac09 merged commit 6f6f7ca into main Dec 16, 2021
@dac09 dac09 deleted the ds-clean-up-fwt-deps branch December 16, 2021 23:24
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Dec 16, 2021
@thedavidprice thedavidprice modified the milestones: next-release, v0.41.0 Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants