-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add gcp-build samples #710
Conversation
Codecov Report
@@ Coverage Diff @@
## master #710 +/- ##
=======================================
Coverage 55.17% 55.17%
=======================================
Files 1 1
Lines 58 58
=======================================
Hits 32 32
Misses 26 26 Continue to review full report at Codecov.
|
Can you add a README.md explaining what the sample does and what is happening when |
<script src="message.min.js" type="text/javascript"></script> | ||
<link rel="stylesheet" href="style.min.css"> | ||
</head> | ||
<body> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add as a visible text to this page that this is loading minified resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto to below comment.
<script src="message.js" type="text/javascript"></script> | ||
<link rel="stylesheet" href="style.css"> | ||
</head> | ||
<body> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add as a visible text to this page that this is loading unminified resources?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this should be obvious based on the different page URLs (/index
vs /index_min
)
appengine/typescript/index.ts
Outdated
|
||
/* tslint:disable:no-console no-var-requires */ | ||
|
||
declare var require: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can do better as a TS example.
This seems to be the copy paste of my little experiement, but does not feel very TS idiomatic: for example, the Express types should be loaded from npm and I would expect the function to use typed objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is largely a copy-paste as I'm not terribly familiar with Typescript 😛
@JustinBeckwith is our resident TS expert, so I'll see what he suggests.
appengine/typescript/app.yaml
Outdated
@@ -0,0 +1,7 @@ | |||
runtime: nodejs8 | |||
|
|||
handlers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove handlers, they are optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you push the commit?
appengine/typescript/README.md
Outdated
|
||
npm install | ||
|
||
or with `yarn`: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personal preference, I would not include yarn
here. Our Node docs say that you can use npm
or yarn
, no need to show everything twice here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed yarn
for brevity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add tests for those samples?
Thanks for adding tests, can you add them to kokoro, too? |
Done - should we merge this first, or the Kokoro stuff? |
🤖 I have created a release \*beep\* \*boop\* --- ### [6.2.7](https://www.github.com/googleapis/nodejs-translate/compare/v6.2.6...v6.2.7) (2021-08-17) ### Bug Fixes * **deps:** google-gax v2.24.1 ([#711](https://www.github.com/googleapis/nodejs-translate/issues/711)) ([87604a3](https://www.github.com/googleapis/nodejs-translate/commit/87604a30f57186c90e8edfe7a3259c41da8c03d2)) * increase timeout for batch translate document ([#708](https://www.github.com/googleapis/nodejs-translate/issues/708)) ([ef154ad](https://www.github.com/googleapis/nodejs-translate/commit/ef154ad287820890a4aaedbf40e91b1cb2f798cc)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
🤖 I have created a release \*beep\* \*boop\* --- ### [6.2.7](https://www.github.com/googleapis/nodejs-translate/compare/v6.2.6...v6.2.7) (2021-08-17) ### Bug Fixes * **deps:** google-gax v2.24.1 ([#711](https://www.github.com/googleapis/nodejs-translate/issues/711)) ([87604a3](https://www.github.com/googleapis/nodejs-translate/commit/87604a30f57186c90e8edfe7a3259c41da8c03d2)) * increase timeout for batch translate document ([#708](https://www.github.com/googleapis/nodejs-translate/issues/708)) ([ef154ad](https://www.github.com/googleapis/nodejs-translate/commit/ef154ad287820890a4aaedbf40e91b1cb2f798cc)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
🤖 I have created a release \*beep\* \*boop\* --- ### [6.2.7](https://www.github.com/googleapis/nodejs-translate/compare/v6.2.6...v6.2.7) (2021-08-17) ### Bug Fixes * **deps:** google-gax v2.24.1 ([#711](https://www.github.com/googleapis/nodejs-translate/issues/711)) ([87604a3](https://www.github.com/googleapis/nodejs-translate/commit/87604a30f57186c90e8edfe7a3259c41da8c03d2)) * increase timeout for batch translate document ([#708](https://www.github.com/googleapis/nodejs-translate/issues/708)) ([ef154ad](https://www.github.com/googleapis/nodejs-translate/commit/ef154ad287820890a4aaedbf40e91b1cb2f798cc)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
🤖 I have created a release \*beep\* \*boop\* --- ### [6.2.7](https://www.github.com/googleapis/nodejs-translate/compare/v6.2.6...v6.2.7) (2021-08-17) ### Bug Fixes * **deps:** google-gax v2.24.1 ([#711](https://www.github.com/googleapis/nodejs-translate/issues/711)) ([87604a3](https://www.github.com/googleapis/nodejs-translate/commit/87604a30f57186c90e8edfe7a3259c41da8c03d2)) * increase timeout for batch translate document ([#708](https://www.github.com/googleapis/nodejs-translate/issues/708)) ([ef154ad](https://www.github.com/googleapis/nodejs-translate/commit/ef154ad287820890a4aaedbf40e91b1cb2f798cc)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
🤖 I have created a release \*beep\* \*boop\* --- ### [6.2.7](https://www.github.com/googleapis/nodejs-translate/compare/v6.2.6...v6.2.7) (2021-08-17) ### Bug Fixes * **deps:** google-gax v2.24.1 ([#711](https://www.github.com/googleapis/nodejs-translate/issues/711)) ([87604a3](https://www.github.com/googleapis/nodejs-translate/commit/87604a30f57186c90e8edfe7a3259c41da8c03d2)) * increase timeout for batch translate document ([#708](https://www.github.com/googleapis/nodejs-translate/issues/708)) ([ef154ad](https://www.github.com/googleapis/nodejs-translate/commit/ef154ad287820890a4aaedbf40e91b1cb2f798cc)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Do not merge - this is missing a (TypeScript) sample.