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

chore: Updates envelop types to v.2.3.0 #6038

Merged
merged 6 commits into from
Aug 4, 2022

Conversation

dthyresson
Copy link
Contributor

Am closing out the Renovate PR #5018 in favor of this due to conflicts.

This PR:

@nx-cloud
Copy link

nx-cloud bot commented Jul 25, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit f7647d9. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 14 targets

Sent with 💌 from NxCloud.

@netlify
Copy link

netlify bot commented Jul 25, 2022

Deploy Preview for redwoodjs-docs canceled.

Name Link
🔨 Latest commit f7647d9
🔍 Latest deploy log https://app.netlify.com/sites/redwoodjs-docs/deploys/62ebcee1c34e9a0009e4ee8b

@dthyresson dthyresson added the release:chore This PR is a chore (means nothing for users) label Jul 25, 2022
@dthyresson dthyresson changed the title chore: Updates envelop types to v.2.30 chore: Updates envelop types to v.2.3.0 Jul 25, 2022
@jtoar
Copy link
Contributor

jtoar commented Jul 26, 2022

@dthyresson did the code changes I made look ok to you? I didn't feel super confident in them so wanted to make sure I didn't break anything.

@dthyresson dthyresson force-pushed the dt-envelop-types-update branch from bdf48b2 to 9f21841 Compare August 1, 2022 15:20
@dthyresson dthyresson requested a review from dac09 August 3, 2022 14:12
@dthyresson
Copy link
Contributor Author

dthyresson commented Aug 3, 2022

@dthyresson did the code changes I made look ok to you? I didn't feel super confident in them so wanted to make sure I didn't break anything.

Yes, but I also had to update the tests for specific types.

Tests are passing and no type warnings, but want to give @dac09 a chance to review was well.

@dac09
Copy link
Contributor

dac09 commented Aug 3, 2022

Hey gents, I had a look through this - the specific changes here look OK to me but I wonder if we're doing something wrong with the plugin types in the first place? I'm confused as to why we need to typecast... it also seems to me that onResolverCalled was the old way of using directives - shouldn't we be using onSchemaChange instead?

I don't want to hold this PR - but we may be hiding some bigger problems here...

@dthyresson
Copy link
Contributor Author

@dac09

shouldn't we be using onSchemaChange instead?

We actually are using onSchemaChange. I think the confusion is we named the custom function that resolves the value the same as "onResolverCalled".

I am renaming to avoid the confusion with the envelop lifecycle event onResolverCalled

@dthyresson
Copy link
Contributor Author

@dac09 See above comment about renaming the function to avoid confusion with onResolverCalled. Renamed to onResolvedValue since it is working with the resolveValue to transform etc.

If good, then I'll merge.

@dac09
Copy link
Contributor

dac09 commented Aug 4, 2022

@dac09 See above comment about renaming the function to avoid confusion with onResolverCalled. Renamed to onResolvedValue since it is working with the resolveValue to transform etc.

Haha I'm still confused DT - but if you're confident, feel free to merge! I've been out of touch of this for a while - so maybe you can just explain this to me at a later time!

@dthyresson dthyresson enabled auto-merge (squash) August 4, 2022 13:51
@dthyresson dthyresson merged commit 9015876 into redwoodjs:main Aug 4, 2022
@redwoodjs-bot redwoodjs-bot bot added this to the next-release milestone Aug 4, 2022
jtoar pushed a commit that referenced this pull request Aug 22, 2022
jtoar pushed a commit that referenced this pull request Aug 22, 2022
@jtoar jtoar modified the milestones: next-release-patch, next-release Sep 2, 2022
@jtoar jtoar added this to the v3.0.0 milestone Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:chore This PR is a chore (means nothing for users)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants