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

dependency upgrades; graphql-tools bug fix, typescript 4.4, deprecated babel-eslint, clean up core/package.json #3371

Merged
merged 23 commits into from
Sep 18, 2021

Conversation

thedavidprice
Copy link
Contributor

@thedavidprice thedavidprice commented Sep 9, 2021

Status:
Everything is working except for a type issue related to MSW cookies.

To Do:

babel-eslint is moved to @babel/eslint-parser

Overview:

Migration:

New parserOptions and adding path to Babel config:

Typescript Release Notes:

Breaking

https://devblogs.microsoft.com/typescript/announcing-typescript-4-4-rc/#use-unknown-catch-variables

Once TypeScript added the unknown type, it became clear that unknown was a better choice than any in catch clause variables for users who want the highest degree of correctness and type-safety, since it narrows better and forces us to test against arbitrary values.

catch(error) may now require catch(error: any)

ESBuild Release Notes:

  • v0.12.27
    • Update JavaScript syntax feature compatibility tables
  • v0.12.26
    • Add --analyze to print information about the bundle
    • Allow require.resolve in non-node builds

@thedavidprice thedavidprice added the release:breaking This PR is a breaking change label Sep 9, 2021
@redwoodjs-bot redwoodjs-bot bot added this to New issues in Current-Release-Sprint Sep 9, 2021
@thedavidprice thedavidprice marked this pull request as draft September 9, 2021 15:12
@thedavidprice thedavidprice added this to the next-release-priority milestone Sep 9, 2021
@redwoodjs-bot redwoodjs-bot bot moved this from New issues to In progress (priority) in Current-Release-Sprint Sep 9, 2021
@thedavidprice thedavidprice moved this from In progress (priority) to v0.37 in Current-Release-Sprint Sep 9, 2021
@thedavidprice thedavidprice linked an issue Sep 9, 2021 that may be closed by this pull request
@dac09
Copy link
Collaborator

dac09 commented Sep 9, 2021

Did you see the breaking changes on graphql-tools @thedavidprice ?

@thedavidprice
Copy link
Contributor Author

@dac09 I did but they didn't apply to us. However, my plan is to wait until #3307 is merged and then revisit.

Only breaking changes so far are TS 4.4 changing error type default to unknown from any (in case of catch(error)).

Lastly, I do want to make sure we try these out with Auth during QA. Might be the right time to add dbAuth to E2E/test-project, which it feels like we'll also need for GraphQL Directives, yes?

@thedavidprice thedavidprice changed the title misc dependency upgrades (graphql-tools, typescript, etc.) dependency upgrades; graphql-tools bug fix, typescript 4.4, deprecated babel-eslint, clean up core/package.json Sep 14, 2021
@thedavidprice
Copy link
Contributor Author

cc @msutkowski

@dac09 I'm upgrading MSW in this PR and running into a TS build error:
https://github.com/redwoodjs/redwood/pull/3371/checks?check_run_id=3594852963#step:5:122

If you recall from a discussion awhile back, here's the source type issue:

set: captureTransform(ctx.set),

It is specifically on the ctx.set due to this breaking change in v0.33.0

Setting a Cookie/Set-Cookie response headers via ctx.set now produces a type violation (#819). Please use ctx.cookie instead.

I tried to diagnose as we first thought this might be due to overly strict typing restrictions. But then you thought the cause might actually be a bug with our typing of captureTransform (or maybe it was GraphQLContext). Any chance you could take a look at this branch and help diagnose?

@thedavidprice thedavidprice marked this pull request as ready for review September 14, 2021 04:58
@msutkowski
Copy link
Contributor

msutkowski commented Sep 15, 2021

cc @msutkowski

@dac09 I'm upgrading MSW in this PR and running into a TS build error:
https://github.com/redwoodjs/redwood/pull/3371/checks?check_run_id=3594852963#step:5:122

If you recall from a discussion awhile back, here's the source type issue:

set: captureTransform(ctx.set),

It is specifically on the ctx.set due to this breaking change in v0.33.0

Setting a Cookie/Set-Cookie response headers via ctx.set now produces a type violation (#819). Please use ctx.cookie instead.

I tried to diagnose as we first thought this might be due to overly strict typing restrictions. But then you thought the cause might actually be a bug with our typing of captureTransform (or maybe it was GraphQLContext). Any chance you could take a look at this branch and help diagnose?

Just do this: set: captureTransform(ctx.set as any),

I don't think there's a reason to try and fight with those types being that they're casted. If you try to do newCtx.set('cookie', 'banana') it'll give you the correct error.

Edit: also, sorry about the delay and radio silence! Hope ya'll are well :)

@thedavidprice
Copy link
Contributor Author

Just do this: set: captureTransform(ctx.set as any),

Well, look at that... Done!

Edit: also, sorry about the delay and radio silence! Hope ya'll are well :)

All good! We're (slowly but surely) eyeing down the v1-rc, which is gonna be 🔥

Thanks as always for the help + support. Hope all's well on your end, too!

@thedavidprice
Copy link
Contributor Author

@dac09 @dthyresson This one's ready to merge. Wanted you both to have a chance to see what's coming down the pipe...

parser: '@babel/eslint-parser',
parserOptions: {
babelOptions: {
configFile: '../../babel.config.js',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note sure about this one... mind walking me through what this does please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing, from the OP:

babel-eslint is moved to @babel/eslint-parser

Overview:

Parser now needs to know location of the babel.config.js or else it throws ('cause can't find otherwise in our directory structure). There's an option to ignore, but after reading docs that didn't seem correct for our case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch @dac09 I'm now checking cwd and using findUp to get the babel.config, which works across Framework and Project now.

Also, I did confirm we must pass the babel config file to the parser. Breaks without it :-(

"lazy-get-decorator": "2.2.0",
"line-column": "1.0.2",
"lodash": "4.17.21",
"lodash-decorators": "6.0.1",
"lru-cache": "6.0.0",
"proxyquire": "2.1.3",
"toml": "3.0.0",
"ts-morph": "11.0.3",
"ts-morph": "12.0.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need resolutions for ts-morph?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thedavidprice
Copy link
Contributor Author

@dac09 just updated babel-preset to get CORE_JS_VERSION from installed package = ✅

This is ready to go

@dthyresson dthyresson self-assigned this Sep 17, 2021
The api package should no longer (I think) have any graphql related dependencies,
@dthyresson
Copy link
Contributor

@thedavidprice I fetched this PR and built, ran tests and ran e2e and all passed.

Apart from the error types question for @dac09 should be good to go.

But, CI has some Cypress issues -- cached node_modules (perhaps due to my committing a package change here vs another commit).

Can you reset this?

@thedavidprice
Copy link
Contributor Author

@dthyresson Yeppers, that was the Cypress Cache error — first time I've seen it although DC says it shows up a lot on his PRs. Most likely resolved in the Yarn v3 PR (I changed the CI caching mechanism). Anyway, I gave it a kick and expect things to pass now.

@dac09 was there a question from DT about error types? Assuming he's talking about this type of change:
https://github.com/redwoodjs/redwood/pull/3371/files#diff-7bcac6982b532d462238a0e73d225686328dbba4357740a47b7d11c320729a02

I recall you saying that's the way to do it. Please confirm.

@dthyresson
Copy link
Contributor

@dac09 was there a question from DT about error types?

@thedavidprice If you already went over this with Danny, then sounds good -- I was just curious about typing Errors.

Then approved and good to merge.

@thedavidprice thedavidprice merged commit 07f4e13 into main Sep 18, 2021
Current-Release-Sprint automation moved this from v0.37 to Done Sep 18, 2021
@thedavidprice thedavidprice deleted the dsp-misc-upgrades-sept-08-2021 branch September 18, 2021 21:27
@thedavidprice thedavidprice modified the milestones: next-release, v0.37.0 Sep 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:breaking This PR is a breaking change
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

After upgrade from 0.35.2 to 0.36.2 GraqhQL comment does not work
4 participants