Adding TypeScript to "Getting Started" #1800
Conversation
| {{< tabs TypeScript JavaScript >}} | ||
|
|
||
| {{< tab >}} | ||
| tsc --init | ||
| npm install --save @opentelemetry/api @opentelemetry/sdk-trace-web @opentelemetry/instrumentation-document-load @opentelemetry/context-zone | ||
| {{< /tab >}} | ||
|
|
||
| {{< tab >}} | ||
| npm init -y | ||
| npm install --save @opentelemetry/api @opentelemetry/sdk-trace-web @opentelemetry/instrumentation-document-load @opentelemetry/context-zone | ||
| ``` | ||
| {{< /tab >}} | ||
|
|
||
| {{< /tabs >}} |
There was a problem hiding this comment.
- Don't use tabs here. The steps to install the required NPM packages will be the same for TS and JS.
- Drop the
--saveargument, sincenpm installsaves the installed packages by default. - Drop the
npm init -y, AFAIK, you don't need it for this simple example app. - The
tsccommand has nothing to do with Installation, which is the subject of this section. Instead, in the next section, mention it as an extra step for TS developers.
There was a problem hiding this comment.
Drop the npm init -y, AFAIK, you don't need it for this simple example app.
I introduced that a while back into the nodejs getting started because I had a node_modules in a parent directory and node is then putting everything there instead of the local directory. Although this is expected behaviour by npm, it's confusing, having a basic package.json fixes that. The alternative is echo {} > package.json.
There was a problem hiding this comment.
I will just keep the npm init -y and tsc --init commands for now
There was a problem hiding this comment.
I introduced that a while back into the nodejs getting started because I had a node_modules in a parent directory and node is then putting everything there instead of the local directory. Although this is expected behaviour by npm, it's confusing, having a basic
package.jsonfixes that.
How likely is it that a first-time user will have a node_modules folder in the parent directory? I'd prefer that we drop the npm init -y command, but I'm not strongly against it.
@kaiwalyakoparkar - all of the other points I made are still relevant. I'll point this out again in my next review.
Thanks @kaiwalyakoparkar! As it turned out this is a much bigger change than I have anticipated in the beginning, so thanks for going back & forth with us on that! |
|
Yes, thank you @kaiwalyakoparkar for your patience and work here |
|
@kaiwalyakoparkar, we are getting closer:) Just found a orphaned space, but beyond that it looks good to me. |
Thank you, resolved it :D |
|
@chalin, any more remarks/concern? |
Sure! I am happy with it, and we can build on top of it, keen to get some end-user feedback how they like it.
+1 and especially, once again thanks @kaiwalyakoparkar , amazing work! |
|
why is EasyCLA pending? @open-telemetry/docs-approvers |
|
Ugh, damn you easycla |
|
/easycla |
|
@austinlparker @cartermp - what was the /cla |
|
Wouldn't know, sadly. |
|
/easycla |
|
Apparently it's meant to be Just to confirm, @kaiwalyakoparkar, you did sign the CLA, right? |
I guess yes (because it I am not getting any option to do either) |
|
@kaiwalyakoparkar can you re-raise the PR? maybe that helps 😞 |
|
Maybe start by rebasing, and doing a force-push, that might kick-start a new EasyCLA check cycle. |
|
close/re-open was a common fix for this kind of stuff but it appears not to have done anything |
|
I will try pushing a blank commit to re-trigger the easycla |
kaiwalyakoparkar
force-pushed
the
kaiwalya-1799
branch
from
75fdad3
to
b7c4fe9
Compare
|
Ooof, maybe this PR is just cursed. @kaiwalyakoparkar would you be willing to submit another PR from this same branch? |
Absolutely. I will close this and raise a new PR |
closes #1799