Skip to content

Add --useReadOnlyType for TS#1205

Merged
trevor-scheer merged 6 commits intoapollographql:masterfrom
mike-marcacci:readonly-ts
May 16, 2019
Merged

Add --useReadOnlyType for TS#1205
trevor-scheer merged 6 commits intoapollographql:masterfrom
mike-marcacci:readonly-ts

Conversation

@mike-marcacci
Copy link
Copy Markdown
Contributor

@mike-marcacci mike-marcacci commented Apr 23, 2019

  • Update CHANGELOG.md* with your change (include reference to issue & this PR)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass

Please note that the checks currently fail due to a flaw in the type definitions for babel-types, which is fixed in this PR. Once that lands, the tests will pass.

@mike-marcacci
Copy link
Copy Markdown
Contributor Author

OK – the upstream PR has been merged, and all tests pass.

@mike-marcacci
Copy link
Copy Markdown
Contributor Author

Bump.

@mike-marcacci
Copy link
Copy Markdown
Contributor Author

Hey, can somebody look at this?

@mike-marcacci
Copy link
Copy Markdown
Contributor Author

Just updated to fix merge conflicts.

Copy link
Copy Markdown
Contributor

@trevor-scheer trevor-scheer 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 awesome, thanks for the PR (and the persistence) @mike-marcacci! Apologies for the delays in getting around to this, we stay pretty busy around here 😉

Just a couple minor things, but overall this is looking good! In line with my comment about preserving the old flag, can we add a test case that still uses the old one?

Comment thread packages/apollo-codegen-flow/src/helpers.ts
Comment thread packages/apollo-codegen-typescript/src/language.ts Outdated
Comment thread packages/apollo/README.md
Comment thread packages/apollo/src/commands/client/codegen.ts
Comment thread packages/apollo/src/commands/client/codegen.ts Outdated
@trevor-scheer
Copy link
Copy Markdown
Contributor

Fun fact, if you update and see some funny business with the tests (azure 8 & 10 failing unexpectedly), feel free to ignore. Tests are a bit flaky for now, and a rerun usually resolves the problem.

These are autogenerated, so these changes are unnecessary.
@mike-marcacci
Copy link
Copy Markdown
Contributor Author

I've made the requested changes, but can no longer run npm i locally on either master or this rebased branch – the lerna hook fails multiple times with:

> lerna run build --stream --scope apollo-env && npm run compile && lerna run build --stream --ignore apollo-env

lerna notice cli v3.13.4
lerna info versioning independent
lerna info filter [ 'apollo-env' ]
lerna info Executing command in 1 package: "npm run build"
apollo-env: > apollo-env@0.5.0 prebuild /Users/mike/Code/apollo-tooling/packages/apollo-env
apollo-env: > npm run clean
apollo-env: > apollo-env@0.5.0 clean /Users/mike/Code/apollo-tooling/packages/apollo-env
apollo-env: > npx rimraf lib
apollo-env: > apollo-env@0.5.0 build /Users/mike/Code/apollo-tooling/packages/apollo-env
apollo-env: > tsc --project ./src/fetch && node clone-types.js && cd src && tsc
apollo-env: copied: src/fetch/fetch.d.ts -> lib/fetch/fetch.d.ts
apollo-env: copied: src/fetch/global.d.ts -> lib/fetch/global.d.ts
apollo-env: copied: src/fetch/index.d.ts -> lib/fetch/index.d.ts
apollo-env: copied: src/fetch/url.d.ts -> lib/fetch/url.d.ts
lerna success run Ran npm script 'build' in 1 package in 2.7s:
lerna success - apollo-env

> apollo-tooling-monorepo@ compile /Users/mike/Code/apollo-tooling
> tsc --build tsconfig.json

error TS2688: Cannot find type definition file for 'apollo-env'.

...so I can't build or run tests locally.

I manually created the snapshot so that the new test passes when run on CircleCI so this PR should be good to go.

(But you'll still probably want to try cloning a fresh copy of the repo to see if there's a more general problem.)

@trevor-scheer
Copy link
Copy Markdown
Contributor

Looks great! An npm run clean && npm i might resolve the build issue you're seeing in development. I added a commit to update the snapshot (looks like it was slightly off), but otherwise this is awesome. I'll merge and release right now 😄

Thanks again for the work on this, including the upstream types fix!

@trevor-scheer trevor-scheer merged commit 528ece0 into apollographql:master May 16, 2019
@mike-marcacci
Copy link
Copy Markdown
Contributor Author

Awesome - Thanks so much for the great review and snappy followup!

@trevor-scheer
Copy link
Copy Markdown
Contributor

Released in apollo@2.12.0 🎉

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.

2 participants