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

Adding TypeScript to "Getting Started" #1891

Merged
merged 5 commits into from
Nov 11, 2022

Conversation

kaiwalyakoparkar
Copy link
Contributor

@kaiwalyakoparkar kaiwalyakoparkar commented Oct 21, 2022

Ref #1800 and #1888
@svrnm @chalin @cartermp kindly take a look

@kaiwalyakoparkar kaiwalyakoparkar requested review from a team as code owners October 21, 2022 17:44
@cartermp
Copy link
Contributor

Looks like you were right about the commits. Thanks for sticking it out like this. Looks like there's failures with respect to the main branch changes now, but once those are taken care of, I can approve.

@kaiwalyakoparkar
Copy link
Contributor Author

Looks like you were right about the commits. Thanks for sticking it out like this. Looks like there's failures with respect to the main branch changes now, but once those are taken care of, I can approve.

Thanks for the information. It's been fixed 👍

@cartermp
Copy link
Contributor

@open-telemetry/javascript-approvers PTAL!

@svrnm
Copy link
Member

svrnm commented Oct 24, 2022

thanks for figthing this true! TY

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Thanks. See inline comments, and questions for @svrnm and other reviewers.

@svrnm
Copy link
Member

svrnm commented Oct 28, 2022

Really want to have this merged soon, @kaiwalyakoparkar can you give the reviews another look?:)

@kaiwalyakoparkar
Copy link
Contributor Author

Really want to have this merged soon, @kaiwalyakoparkar can you give the reviews another look?:)

Sure, I will commit the required changes soon

Co-authored-by: Patrice Chalin <chalin@users.noreply.github.com>
@svrnm
Copy link
Member

svrnm commented Nov 11, 2022

@open-telemetry/javascript-approvers please give this another look :-)

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

Overall LGTM % some nits.

{{< ot-tabs TypeScript JavaScript >}}

{{< ot-tab lang="html">}}
<script type="module" src="document-load.ts"></script>
Copy link
Member

Choose a reason for hiding this comment

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

I don't think browsers are able to load typescript source codes directly. For both JavaScript and TypeScript projects, here the src attribute value should be document-load.js (the output of tsc compilation).

Copy link
Member

Choose a reason for hiding this comment

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

If I remember correctly parel (the build tool used in the example) will infer the type of the file from that and then do a proper compile and replacement. @kaiwalyakoparkar have you run through that example with TS and can verify, if not let's try this out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @svrnm I did run these files and can confirm that the output I received is according to the docs

Copy link
Member

Choose a reason for hiding this comment

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

IMO it's quite confusing as parcel isn't mentioned until after the code samples. If you want to depend on bundler-specific behavior I would at least call it out at the top of the document that the samples will require some particular build process or dependency.

Copy link
Member

Choose a reason for hiding this comment

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

@dyladan fair point! That one is on me, I wrote that tutorial initially. Let's have this in a separate (follow-up) issue

Copy link
Contributor

Choose a reason for hiding this comment

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

I filed #1989 for this issue

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Code samples look good to me. I share @legendecas reservation about the ts extension when it isn't clear that this is enabled by build tooling

{{< ot-tabs TypeScript JavaScript >}}

{{< ot-tab lang="html">}}
<script type="module" src="document-load.ts"></script>
Copy link
Member

Choose a reason for hiding this comment

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

IMO it's quite confusing as parcel isn't mentioned until after the code samples. If you want to depend on bundler-specific behavior I would at least call it out at the top of the document that the samples will require some particular build process or dependency.

@cartermp
Copy link
Contributor

I will go ahead and merge, then we can address #1989 orthogonally, since it is all that remains. Thanks all, and especially @kaiwalyakoparkar for your diligence and patience across several PRs!

@cartermp cartermp merged commit 9875694 into open-telemetry:main Nov 11, 2022
@kaiwalyakoparkar kaiwalyakoparkar deleted the kaiwalya-1799-refined branch November 12, 2022 06:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants