Skip to content
This repository was archived by the owner on Mar 11, 2026. It is now read-only.

refactor(ts): improve typescript types#309

Merged
JustinBeckwith merged 1 commit intogoogleapis:masterfrom
JustinBeckwith:ts3
Dec 19, 2018
Merged

refactor(ts): improve typescript types#309
JustinBeckwith merged 1 commit intogoogleapis:masterfrom
JustinBeckwith:ts3

Conversation

@JustinBeckwith
Copy link
Copy Markdown
Contributor

@JustinBeckwith JustinBeckwith commented Nov 7, 2018

No description provided.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 7, 2018
@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 7, 2018 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@abcd75d). Click here to learn what that means.
The diff coverage is 58.97%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #309   +/-   ##
=========================================
  Coverage          ?   89.96%           
=========================================
  Files             ?       14           
  Lines             ?      658           
  Branches          ?       75           
=========================================
  Hits              ?      592           
  Misses            ?       36           
  Partials          ?       30
Impacted Files Coverage Δ
src/sink.ts 89.18% <ø> (ø)
src/entry.ts 89.65% <100%> (ø)
src/index.ts 96.66% <100%> (ø)
src/log.ts 79.26% <36%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update abcd75d...af0d914. Read the comment docs.

@JustinBeckwith JustinBeckwith added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 7, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 7, 2018
ofrobots
ofrobots reviewed Nov 7, 2018
View reviewed changes
Comment thread src/entry.ts Outdated
@ofrobots ofrobots mentioned this pull request Nov 7, 2018
Merged
ofrobots
ofrobots suggested changes Nov 7, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

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

Given that this is getting approvals, blocking this so that alternative #314 can be evaluated.

@JustinBeckwith
Copy link
Copy Markdown
Contributor Author

JustinBeckwith commented Nov 12, 2018

ok @ofrobots ready for another look :)

ofrobots
ofrobots reviewed Nov 12, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

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

I need to finish this off when I have some more time.. I did add some comments already that are actionable.

The other thing is that once this lands, we need to ship the generated logging.d.ts. We are exporting interface types from it. We shouldn't need to ship logging.js as long as we are only referencing the interface rather than concrete types.

Comment thread package.json Outdated
Comment thread src/entry.ts Outdated
@JustinBeckwith
Copy link
Copy Markdown
Contributor Author

JustinBeckwith commented Nov 16, 2018

@ofrobots one more time 😆

@ofrobots
Copy link
Copy Markdown
Contributor

ofrobots commented Nov 17, 2018 •
edited
Loading

The proto directory still needs to be copied to build and added to the files section of package.json.

ofrobots
ofrobots reviewed Nov 18, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

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

Still looking.. but have some comments...

Comment thread src/sink.ts
Comment thread src/log.ts Outdated
Comment thread src/log.ts Outdated
Comment thread src/log.ts
Comment thread src/log.ts
Comment thread src/log.ts
Comment thread src/log.ts
Comment thread src/log.ts
Comment thread src/log.ts
Comment thread src/log.ts
ofrobots
ofrobots reviewed Nov 18, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

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

Looking great. Some more things to address:

So, at this point we don't need use proto/logging.js at runtime, and we don't need to ship it (it is rather large).

Can you drop that file from this PR? You will have to replace the imports of proto/logging to imports of proto/logging.d.

Comment thread package.json
Comment thread src/entry.ts Outdated
@JustinBeckwith
Copy link
Copy Markdown
Contributor Author

JustinBeckwith commented Dec 3, 2018

@ofrobots oooook. One more time.

@ofrobots
Copy link
Copy Markdown
Contributor

ofrobots commented Dec 3, 2018

@JustinBeckwith can you address these:

So, at this point we don't need use proto/logging.js at runtime, and we don't need to ship it (it is rather large).

Can you drop that file from this PR? You will have to replace the imports of proto/logging to imports of proto/logging.d.

@JustinBeckwith
Copy link
Copy Markdown
Contributor Author

JustinBeckwith commented Dec 15, 2018

@ofrobots done 🎉

@ofrobots
Copy link
Copy Markdown
Contributor

ofrobots commented Dec 17, 2018 •
edited
Loading

Awesome. May I suggest this way of doing it instead:

-"proto:logging": "mkdir -p proto && pbjs -t static-module -w commonjs -p node_modules/google-proto-files -o proto/logging.js google/logging/v2/logging.proto && pbts -o proto/logging.d.ts proto/logging.js"
+"proto:logging": "mkdir -p proto && pbjs -t static-module -w commonjs -p node_modules/google-proto-files google/logging/v2/logging.proto | pbts -o proto/logging.d.ts -"

ofrobots
ofrobots approved these changes Dec 17, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

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

LGTM once comment addressed

@JustinBeckwith JustinBeckwith merged commit a55c43e into googleapis:master Dec 19, 2018
@snyk-bot snyk-bot mentioned this pull request Mar 1, 2020
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@ofrobots ofrobots ofrobots approved these changes

@jkwlui jkwlui jkwlui approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@JustinBeckwith @ofrobots @jkwlui @googlebot @kokoro-team