Skip to content

feat!: update the build to use api-extractor types #383

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Nov 10, 2021

Conversation

matthewrobertson
Copy link
Member

@matthewrobertson matthewrobertson commented Nov 10, 2021

This commit updates our build config to use the types generated by the
API extractor. This prevents us exposing private implementation details
in our type declarations that are not marked @public

I also refactored our build scripts a bit so we can ensure api-extractor runs before we publish:

  1. removed predocs
  2. replaced clean with the first part of compile that deletes the /build dir
  3. created a new build that runs clean, compile, docs
  4. made prepare and alias for build

package.json Outdated
@@ -21,7 +21,7 @@
"conformance": "./run_conformance_tests.sh",
"check": "gts check",
"clean": "gts clean",
"compile": "rm -rf ./build && tsc -p .",
"compile": "rm -rf ./build && tsc -p . && api-extractor run --local --verbose",
Copy link
Contributor

Choose a reason for hiding this comment

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

While it's great to add the API extractor in our scripts, I don't suggest we make the tool run in the TS compilation script.

Moreover, this looks like a copy of && npm run docs.

We already have a predocs script that runs compile.

Copy link
Member Author

Choose a reason for hiding this comment

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

the reason I didn't make it && npm run docs is because we have a predocs that runs compile so it would have created a cycle...

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. However, we're repeating ourselves though with api-extractor run --local --verbose.

There are a few ways to avoid this. For example, creating a separate command like api-extractor and running the command there.

However, I'm not sure we want to run the doc extractor when we compile.

For example, it looks like this breaks our watch command:

npm run watch

> @google-cloud/functions-framework@1.10.1 watch
> npm run compile -- --watch


> @google-cloud/functions-framework@1.10.1 compile
> rm -rf ./build && tsc -p . && api-extractor run --local --verbose "--watch"


api-extractor 7.18.16  - https://api-extractor.com/

usage: api-extractor run [-h] [-c FILE] [-l] [-v] [--diagnostics]
                         [--typescript-compiler-folder PATH]
                         
api-extractor run: error: Unrecognized arguments: --watch.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm yeah, I think the key think is I want to make sure we run api-extractor before we publish (so in prepare). I will try to refactor this a bit

Copy link
Member Author

Choose a reason for hiding this comment

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

OK I refactored the scripts a bit:

  1. removed predocs
  2. replaced clean with the first part of compile
  3. created a new build that runs clean, compile, docs
  4. made prepare and alias for build

package.json Outdated
@@ -21,7 +21,7 @@
"conformance": "./run_conformance_tests.sh",
"check": "gts check",
"clean": "gts clean",
"compile": "rm -rf ./build && tsc -p .",
"compile": "rm -rf ./build && tsc -p . && api-extractor run --local --verbose",
Copy link
Contributor

Choose a reason for hiding this comment

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

Right. However, we're repeating ourselves though with api-extractor run --local --verbose.

There are a few ways to avoid this. For example, creating a separate command like api-extractor and running the command there.

However, I'm not sure we want to run the doc extractor when we compile.

For example, it looks like this breaks our watch command:

npm run watch

> @google-cloud/functions-framework@1.10.1 watch
> npm run compile -- --watch


> @google-cloud/functions-framework@1.10.1 compile
> rm -rf ./build && tsc -p . && api-extractor run --local --verbose "--watch"


api-extractor 7.18.16  - https://api-extractor.com/

usage: api-extractor run [-h] [-c FILE] [-l] [-v] [--diagnostics]
                         [--typescript-compiler-folder PATH]
                         
api-extractor run: error: Unrecognized arguments: --watch.

@matthewrobertson matthewrobertson force-pushed the use-extracted-types branch 2 times, most recently from 3101166 to 644447d Compare November 10, 2021 02:15
Copy link
Contributor

@grant grant left a comment

Choose a reason for hiding this comment

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

1 requested change.

I didn't test to see if there were conflicting commands.

Otherwise lgtm.

package.json Outdated
"conformance": "./run_conformance_tests.sh",
"check": "gts check",
"clean": "gts clean",
"compile": "rm -rf ./build && tsc -p .",
"clean": "rm -rf ./build",
Copy link
Contributor

Choose a reason for hiding this comment

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

There shouldn't be a need to replace gts with a manual rimraf of the build folder declared in our tsconfig.

gts clean is the expected clean command we use in client libs and elsewhere in this library. It also provides better output via gts.

This change is also a little tangential to the api-extractor change.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed it because gts clean doesn't work on my machine:

❯ npm run clean

> @google-cloud/functions-framework@1.10.1 clean /usr/local/google/home/mattrobertson/gcf/functions-framework-nodejs
> gts clean

version: 16
Error: Error: /usr/local/google/home/mattrobertson/gcf/functions-framework-nodejs/tsconfig.json
Error: /usr/local/google/home/mattrobertson/gcf/functions-framework-nodejs/gts/tsconfig-google.json
ENOENT: no such file or directory, open '/usr/local/google/home/mattrobertson/gcf/functions-framework-nodejs/gts/tsconfig-google.json'
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! @google-cloud/functions-framework@1.10.1 clean: `gts clean`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the @google-cloud/functions-framework@1.10.1 clean script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /usr/local/google/home/mattrobertson/.npm/_logs/2021-11-10T17_43_50_564Z-debug.log

@grant does it work for you right now?

Copy link
Member Author

Choose a reason for hiding this comment

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

looks like our tsconfig should actually be extending ./node_modules/gts/tsconfig-google.json:

https://github.com/googleapis/nodejs-pubsub/blob/main/tsconfig.json#L2

I wonder how tsc is even working now...

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. It looks like that's a different issue.

It looks like this expects a global gts or something maybe changed when this was first created with linking. We can change it to the local gts with tsconfig:

"extends": "./node_modules/gts/tsconfig-google.json",

This config looks the same as other client libs.

We probably shouldn't inline rm rf because one of our scripts doesn't work as expected.

This commit updates our build config to use the types generated by the
API extractor. This prevents us exposing private implementation details
in our type declarations that are not marked `@public`
@matthewrobertson matthewrobertson merged commit 752c49c into master Nov 10, 2021
@release-please release-please bot mentioned this pull request Nov 10, 2021
@matthewrobertson matthewrobertson deleted the use-extracted-types branch December 7, 2021 20:24
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants