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

Add gcp-build samples #710

Merged
merged 14 commits into from
Aug 21, 2018
Merged

Add gcp-build samples #710

merged 14 commits into from
Aug 21, 2018

Conversation

ace-n
Copy link
Contributor

@ace-n ace-n commented Aug 14, 2018

Do not merge - this is missing a (TypeScript) sample.

@ace-n ace-n added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 14, 2018
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 14, 2018
@codecov
Copy link

codecov bot commented Aug 14, 2018

Codecov Report

Merging #710 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           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.

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

@steren
Copy link
Contributor

steren commented Aug 14, 2018

Can you add a README.md explaining what the sample does and what is happening when gcp-build is run?

<script src="message.min.js" type="text/javascript"></script>
<link rel="stylesheet" href="style.min.css">
</head>
<body>
Copy link
Contributor

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.

Copy link
Contributor Author

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>
Copy link
Contributor

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?

Copy link
Contributor Author

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)

@ace-n ace-n requested a review from jmdobry August 14, 2018 21:31

/* tslint:disable:no-console no-var-requires */

declare var require: any;
Copy link
Contributor

@steren steren Aug 14, 2018

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.

Copy link
Contributor Author

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.

@@ -0,0 +1,7 @@
runtime: nodejs8

handlers:
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

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?

@ace-n ace-n requested review from jkwlui and removed request for JustinBeckwith August 15, 2018 18:56
@ace-n ace-n removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 16, 2018

npm install

or with `yarn`:
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed yarn for brevity.

Copy link
Contributor

@fhinkel fhinkel left a 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?

@fhinkel
Copy link
Contributor

fhinkel commented Aug 20, 2018

Thanks for adding tests, can you add them to kokoro, too?

@ace-n
Copy link
Contributor Author

ace-n commented Aug 21, 2018

Done - should we merge this first, or the Kokoro stuff?

@ace-n ace-n added the kokoro:run Add this label to force Kokoro to re-run the tests. label Aug 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants