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

New contributor workflow #1792

Merged
merged 15 commits into from
Feb 23, 2021
Merged

Conversation

dac09
Copy link
Collaborator

@dac09 dac09 commented Feb 13, 2021

Introduces new contributor workflow (without removing old copy:watch flow)

In order to contribute from a project now, you simply need to run

yarn rwt link [RW_PATH]

This will ask to setup your workspaces, and create the symlink for you. It willl also build and watch the redwood repo, so no need to build using another separate terminal session.

Once you're done, and you want to restore, you run

yarn rwt restore

Need input

  • Is rwt link intuitive? Or should we call it something else? This is meant to be like npm link, yarn link
  • Improve the messaging, maybe use boxen to explain?
  • rwt restore -> rwt unlink? I don't like unlink for some reason
  • On my machine, in the redwood repo it creates a symlink inside packages. So packages/packages - no idea why!

Other benefits

  • I think it consumes less cpu (one less watcher)
  • You can install / yarn rw setup stuff, without breaking the contribution workflow
  • Cleans up the redwood-tools.js file to make it easier to wrangle the code
  • Fixes infinite loop that sometimes happens building eslint-plugin
  • Fixes configs (webpack, etc.) not being available on the project until save. Since it's using symlinks, we don't have to worry about rebuilding!
  • We don't need to worry about manually installing new dependencies when using canary (just remember to yarn in your redwood repo)

Preview of what it looks like:
https://s.tape.sh/NSEQV6FH

@github-actions
Copy link

github-actions bot commented Feb 13, 2021

📦 PR Packages

Click to Show Package Download Links

https://rw-pr-redwoodjs-com.s3.amazonaws.com/1792/create-redwood-app-0.25.1-82ea6f7.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1792/redwoodjs-api-0.25.1-82ea6f7.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1792/redwoodjs-api-server-0.25.1-82ea6f7.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1792/redwoodjs-auth-0.25.1-82ea6f7.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1792/redwoodjs-cli-0.25.1-82ea6f7.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1792/redwoodjs-core-0.25.1-82ea6f7.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1792/redwoodjs-dev-server-0.25.1-82ea6f7.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1792/redwoodjs-eslint-config-0.25.1-82ea6f7.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1792/redwoodjs-eslint-plugin-redwood-0.25.1-82ea6f7.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1792/redwoodjs-forms-0.25.1-82ea6f7.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1792/redwoodjs-internal-0.25.1-82ea6f7.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1792/redwoodjs-prerender-0.25.1-82ea6f7.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1792/redwoodjs-router-0.25.1-82ea6f7.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1792/redwoodjs-structure-0.25.1-82ea6f7.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1792/redwoodjs-testing-0.25.1-82ea6f7.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1792/redwoodjs-web-0.25.1-82ea6f7.tgz

Install this PR by running yarn rw upgrade --pr 1792:0.25.1-82ea6f7

@jtoar
Copy link
Contributor

jtoar commented Feb 13, 2021

@dac09 super excited to try this out! Confirming the steps are:

  1. create a new Redwood app and upgrade to this PR with yarn rw upgrade --pr 1792:0.25.0-6b66c9f
  2. still in that redwood app, run yarn rwt link [RW_PATH]
  3. say yes to the prompt
  4. see edits from your local copy of the redwood repo live in the redwood app?

Here's a tape of me trying out a new setup command I'm working on. The repo doesn't seem to pick it up. Did I miss anything obvious? https://s.tape.sh/aIoTMMq2

@dac09
Copy link
Collaborator Author

dac09 commented Feb 13, 2021

Your process Looks fine to me Dom! Not sure why it's not coming through. Could you try:
a) yarn in the root of your project
b) Is there a soft link called "redwood" in the root of your project?

@jtoar
Copy link
Contributor

jtoar commented Feb 13, 2021

Tried yarn in the root and still no luck, but that soft link is there. Maybe it has to do with adding a new CLI command? I can try making changes to another part of the framework, like the router, and see if they propagate.

image

@dac09
Copy link
Collaborator Author

dac09 commented Feb 14, 2021

Weird. I've been using this to test itself (does that make any sense?!) Probably missed something. I'll try it out more on Monday!

@dac09
Copy link
Collaborator Author

dac09 commented Feb 15, 2021

Ah, I can see why. When you build using the PR flow, it adds @rw/cli to your dev dependencies. For some reason this takes precedence over your locally linked packages. Once I removed this dependency form package.json and yarn'd, it all came through fine :) https://s.tape.sh/zfsZSxa6

Tldr; this is difficult to test with the PR flow. Steps

  1. Run upgrade PR command
  2. Run yarn rwt link [PATH] once, so it adds the symlink, etc. for you
  3. Remove @redwoodjs/cli from your dev dependencies in package.json (created by the pr upgrade thingy)
  4. If you don't have this branch checked out on your redwood repo, run: chmod +x node_modules/.bin/rwt && chmod +x node_modules/.bin/rw
  5. yarn rwt link [PATH] again.

If/once this is part of the redwood releases, you'll only need to run step 2.

Edit: updated based on comments below

@jvanbaarsen
Copy link
Contributor

@dac09 This is great! Would make the experience so much better :) Are there certain cases you'd like people to test?

@jtoar
Copy link
Contributor

jtoar commented Feb 15, 2021

@dac09 Those steps worked great! One thing I wanted to add: at the end you'll most likely have to make the rw binary executable again using chmod +x ./node_modules/.bin/rw.

@jvanbaarsen
Copy link
Contributor

@dac09 Just tested this locally works great! It's important to keep in mind the devDependency indeed (Would this be a problem when people upgraded their local repo with a PR upgrade?) One thing to note as @jtoar said, every time the thing did a rebuild you need to chmod the rw binary.

@dac09
Copy link
Collaborator Author

dac09 commented Feb 15, 2021

I definitely added the chmod inside the watcher... so everytime cli, apiserver and devserver builds, they get chmodded. I wonder why they hadn't in this case?

@jvanbaarsen
Copy link
Contributor

jvanbaarsen commented Feb 15, 2021

@dac09 You chmod dist/index.js, not the actual rw executable. Can that be the problem?
--edit: Nvm.. Didn't look correctly

@dac09
Copy link
Collaborator Author

dac09 commented Feb 15, 2021

OH! I know why. It's because the changes to the build folder also need to be checked out in your local redwood repo LOL.

This is so meta!

So without the changes to the build process in your local repo, it won't chmod automatically :)

@jvanbaarsen
Copy link
Contributor

@dac09 I was just about to post that 🙈 I'll double check this assumption with checkout out this PR in my RW reop.

@dac09
Copy link
Collaborator Author

dac09 commented Feb 15, 2021

Headsup 👒, I just rebased my branch. Now added --clean flag.

@jvanbaarsen, @jtoar any chance you guys can see why

On my machine, in the redwood repo it creates a symlink inside packages. So packages/packages - no idea why!

@jvanbaarsen
Copy link
Contributor

@dac09 Your assumption was right! After checking out this PR as my RW base it works like a charm :) Also seeing the packages/packages thing though

@jvanbaarsen
Copy link
Contributor

@dac09 Can it be that because you're linking packages into ./redwood and you then build again it kinda gets in a loop? So it keeps trying to create a folder packages inside of packages? (But since you cannot have a symlink in another symlink you just get one?)

@dac09
Copy link
Collaborator Author

dac09 commented Feb 15, 2021

I don't htink so @jvanbaarsen, the build is running in the redwood repo, not the actual project (cwd is set to frameworkPath).

You can also do each of those steps manually, the function really isn't doing anything special.

@jvanbaarsen
Copy link
Contributor

jvanbaarsen commented Feb 15, 2021

@dac09 What is the reason packages/* is added to the project package.json?

--edit: Nevermind, that is something new that is added in the newest release

@dac09
Copy link
Collaborator Author

dac09 commented Feb 18, 2021

I can add a flag so you can run yarn rwt l --watch false ? @jtoar

@Tobbe
Copy link
Member

Tobbe commented Feb 18, 2021

I ran into issues last time I tried cpw in Windows. Something about files being in use by another process. But when I tried yesterday with link I didn't get that error.

Should the flag be --no-watch?

@dac09
Copy link
Collaborator Author

dac09 commented Feb 18, 2021

Although I've added no-xxx as flags before, I think that its better than we just use a boolean to control these flags. With the defaults being true, for sensible defaults (e.g. clean, watch).

@dac09
Copy link
Collaborator Author

dac09 commented Feb 18, 2021

Btw - I've added to the added benefits section, I've found more cases where this can be useful!

…rib-workspaces

* 'main' of github.com:redwoodjs/redwood:
  Prerender Part 1 & 2: Render landing/marketing/static pages + Private page whileLoading (redwoodjs#1641)
@jvanbaarsen
Copy link
Contributor

@Tobbe I believe. if you have a boolean value for watch, you get --no-watch for free.

@jtoar Yeah for me it is make a change in the Redwood repo, switch to test project and see if the change worked, the less steps the better for me :)

@Tobbe
Copy link
Member

Tobbe commented Feb 18, 2021

@Tobbe I believe. if you have a boolean value for watch, you get --no-watch for free.

Yeah, I think you're right, yargs does that, doesn't it? 👍

@dac09
Copy link
Collaborator Author

dac09 commented Feb 18, 2021

@Tobbe I believe. if you have a boolean value for watch, you get --no-watch for free.

#TIL!

@jvanbaarsen
Copy link
Contributor

@Tobbe Yes! I actually got tripped up by this after someone created an issue with the --no-watch flag for the rw test command. I was about to open a PR to document that behaviour until I noticed it is something that is given for free by yargs.

@jvanbaarsen
Copy link
Contributor

@dac09 What is currently holding us back in merging this? Is there anything I can do to speed this up a bit more?

@dac09
Copy link
Collaborator Author

dac09 commented Feb 20, 2021

Just trying to minimise changes going into the release :)

@dac09
Copy link
Collaborator Author

dac09 commented Feb 20, 2021

@jvanbaarsen I also noticed this failing when I try it using canary. Something about the svgo binary not found (introduced in prerender image handling with Babel-plugin-inline-svg). Could you please see if you face the same problem?

@jvanbaarsen
Copy link
Contributor

@dac09 I was just about to test this and then realised I'm unsure what you mean with Canary? You either use Canary or the PR release right? There is no way to use both of them at the same time?

…rib-workspaces

* 'main' of github.com:redwoodjs/redwood:
  Lint the create-redwood-app template. (redwoodjs#1822)
  Update typescript lint rule for no-unused-vars (redwoodjs#1808)
  Router: Fix TS types (redwoodjs#1823)
  Generate globals for routes (redwoodjs#1744)
  Ethereum auth update to v0.2.1 (redwoodjs#1807)
  Better messages when no routes marked with prerender (redwoodjs#1821)
  Feature/prerender image support (redwoodjs#1721)
  Fix entry js | Make index a little more readable, as it confuses people (redwoodjs#1819)
  use 'prisma' in place of '@prisma/cli' (redwoodjs#1800)
@dac09
Copy link
Collaborator Author

dac09 commented Feb 22, 2021

Hey @jvanbaarsen sorry for not being clear! I've just updated the branch (merging main into this).

What we'll need to test is the latest PR build (once its updated).

@dac09
Copy link
Collaborator Author

dac09 commented Feb 22, 2021

@thedavidprice think this is ready for prime time. Shall we get it into 0.26? Docs are covered here: #1803

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.

I'm not able to get this working using the instructions provided by @dac09 in this comment.

I can get the initial yarn rwt link ... to run. But not commands. I'm also seeing endless build:watch process:

Screen.Recording.2021-02-22.at.4.19.27.PM.mov

@Tobbe
Copy link
Member

Tobbe commented Feb 23, 2021

@thedavidprice I think you need to have your rw framework checked out to @dac09's branch (At least that's what I had to do to fix the build loop)

git checkout -b dac09-feature/contrib-workspaces main
git pull https://github.com/dac09/redwood.git feature/contrib-workspaces

@dac09
Copy link
Collaborator Author

dac09 commented Feb 23, 2021

@thedavidprice as Tobbe said, the infinite loop is caused by an existing bug in the build process. (Try this on any other branch, yarn build:clean && yarn build:watch in the redwood repo. The infinite loop is also fixed on this PR, so unless you have this branch checked out in your redwood repo, it'll infinite loop.

@thedavidprice thedavidprice merged commit dd17d24 into redwoodjs:main Feb 23, 2021
Ops & Contributing Improvements automation moved this from In progress to Done Feb 23, 2021
@thedavidprice thedavidprice added this to the next release milestone Feb 23, 2021
@dac09 dac09 deleted the feature/contrib-workspaces branch February 23, 2021 17:25
dac09 added a commit to dac09/redwood that referenced this pull request Feb 24, 2021
… fix/dm-babel-register

* 'fix/dm-babel-register' of github.com:dac09/redwood:
  Remove bundlesize dependency (redwoodjs#1844)
  upgrade execa; fix test.js cwd (redwoodjs#1846)
  Make ESLint configuration aware of "env." (redwoodjs#1827)
  Use createMany in Seed database example (redwoodjs#1776)
  Docs: Update contributor workflow to yarn rwt link (redwoodjs#1803)
  fix(page-loader): Adds types for pageLoadingContext | Fix for pageLoader during prerender (redwoodjs#1832)
  Minify CSS in production builds (redwoodjs#1697)
  New contributor workflow (redwoodjs#1792)
  Improve/template and auth setup tests (redwoodjs#1834)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants