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

Static typing for cells #2208

Merged
merged 4 commits into from
Apr 16, 2021
Merged

Static typing for cells #2208

merged 4 commits into from
Apr 16, 2021

Conversation

corbt
Copy link
Contributor

@corbt corbt commented Apr 4, 2021

This PR is the first step towards implementing #2205. It allows users to declare the prop types their cells expect.

It also renames withCell to createCell, for the reasons I outlined in #2205 (comment). createCell feels like a more parsimonious name to me but happy to back this change out if others disagree.

It appears that the babel transform for wrapping cells already checks for a default export and bails out if one exists, so this shouldn't require any changes there.

The major remaining task is updating the cell generator to add a export default createCell line at the end of each cell in Typescript. I'd appreciate more input on whether this is a change we're fine making on both the TS and JS sides, or whether we'll need two templates. Do we have multiple templates anywhere else?

@github-actions
Copy link

github-actions bot commented Apr 4, 2021

📦 PR Packages

Click to Show Package Download Links

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

Install this PR by running yarn rw upgrade --pr 2208:0.30.0-bc9f389

@thedavidprice thedavidprice requested review from peterp and dac09 April 4, 2021 05:38
This PR is the first step towards implementing redwoodjs#2205. It allows users to declare the prop types their cells expect.

It also renames `withCell` to `createCell`, for the reasons I outlined in redwoodjs#2205 (comment). `createCell` feels like a more parsimonious name to me but I can back that change out if others disagree.

It appears that the babel transform for wrapping cells already checks for a default export and bails out if one exists, so this shouldn't require any changes there.

The major remaining task is updating the cell generator to add a `export default createCell` line at the end of each cell in Typescript. I'd appreciate more input on whether this is a change we're fine making on both the TS and JS sides, or whether we'll need two templates. Do we have multiple templates anywhere else?
Copy link
Contributor

@peterp peterp left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@peterp peterp added this to the next release milestone Apr 4, 2021
@dac09 dac09 added release:breaking This PR is a breaking change update-docs labels Apr 5, 2021
@thedavidprice thedavidprice modified the milestones: next release, hold-merge Apr 6, 2021
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.

Hi all! @mojombo has asked us to HODL merging this PR until he's able to dig in later this week / early next. Since he's out this week, he and I decided to not include anything with potential API changes in v0.29 release.

I've added it to the following spring and created a milestone "hold-merge".

Sound good?

@thedavidprice
Copy link
Contributor

Update: Core Team had a great discussion about this earlier today. @peterp will be following up with the next steps as we'd like to get this into v0.30 (next release).

We'll loop in Tom one more time once we finalize a the discussion topics from earlier today.

Thanks again!

@corbt
Copy link
Contributor Author

corbt commented Apr 14, 2021

Awesome, will wait to hear from @peterp !

@peterp
Copy link
Contributor

peterp commented Apr 16, 2021

I've updated this so that it's no longer a breaking change - we're not 100% confident that people aren't using the withCell export, so I've added a deprecation notice that we'll remove it in v0.31..

One of the concerns that we also have about this approach is that if a user declares beforeQuery in the cell file, they need to remember to pass that to createCell.

I think this may or may-not be a concern since if the user doesn't export beforeQuery they'll get warned about an unused variable. Either way we're going to try and make this feel a bit more magical by explicitly warning them (via eslint), or automatically updating the file... but we'll figure that out :)

@peterp peterp removed the release:breaking This PR is a breaking change label Apr 16, 2021
packages/web/src/index.ts Outdated Show resolved Hide resolved
@dac09 dac09 dismissed thedavidprice’s stale review April 16, 2021 14:28

UnHODLing. Peter P is on it!

Co-authored-by: Daniel Choudhury <dannychoudhury@gmail.com>
@peterp peterp merged commit 0730651 into redwoodjs:main Apr 16, 2021
@corbt
Copy link
Contributor Author

corbt commented Apr 16, 2021

Wohoo! Excited this is in. :)

dac09 added a commit to dac09/redwood that referenced this pull request Apr 21, 2021
…erve-web

* 'main' of github.com:redwoodjs/redwood: (40 commits)
  Support generating typescript cells and pages | Autodetect ts project (redwoodjs#2279)
  create-redwood-app messages: moved app start commands to end (redwoodjs#2278)
  Add import type to configuration files (redwoodjs#2214)
  Bump @reach/skip-nav from 0.13.2 to 0.15.0 (redwoodjs#2237)
  Adding Setup Deploy Render Command (redwoodjs#2099)
  Disable role linting in Routes (redwoodjs#2318)
  v0.30.1 (redwoodjs#2322)
  teardown should attempt to delete dbName table (redwoodjs#2083)
  Restore @storybook/addon-a11y (redwoodjs#2309)
  fix(auth): Implement automatic token refresh on supported providers (redwoodjs#2277)
  fix(cli): move api-server dep from api to cli (redwoodjs#2307)
  Static typing for cells (redwoodjs#2208)
  Recommended Babel package upgrades (dependabot) (redwoodjs#2255)
  v0.30.0 (redwoodjs#2301)
  upgrade Prisma v2.21.0 (redwoodjs#2273)
  Further improvements to CONTRIBUTING.md (redwoodjs#2261)
  Adds better messages for rwt link | Watcher does not exist on build failure | Only remove node_modules after a succesful framework build (redwoodjs#2269)
  Update named param types in router readme (redwoodjs#2262)
  Bump core-js from 3.6.5 to 3.10.1 (redwoodjs#2243)
  Fix: webpack optimizations for JS (redwoodjs#2235)
  ...
@dac09 dac09 mentioned this pull request Apr 22, 2021
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.

4 participants