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 proper TypeScript build logic #417

Merged
merged 15 commits into from
May 7, 2021

Conversation

willarmiros
Copy link
Contributor

Issue #, if available:
#411

Description of changes:
These changes allow the repo to conform to TypeScript best practices of not committing transpiled code to source control as we were doing before. Now, we have a compile target on the core package which should be run before testing or publishing the package. It transpiles the TypeScript in the source package, and copies the transpiled JavaScript and all other non-TS source files to a dist directory, which will ultimately be tested or vended to customers.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov-commenter
Copy link

codecov-commenter commented May 6, 2021

Codecov Report

❗ No coverage uploaded for pull request base (aws-v3-support@bcde5c2). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@                Coverage Diff                @@
##             aws-v3-support     #417   +/-   ##
=================================================
  Coverage                  ?   55.23%           
=================================================
  Files                     ?       37           
  Lines                     ?     2714           
  Branches                  ?        0           
=================================================
  Hits                      ?     1499           
  Misses                    ?     1215           
  Partials                  ?        0           

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 bcde5c2...86c93d6. Read the comment docs.

@@ -4,7 +4,7 @@ on:
push:
branches: [master]
pull_request:
branches: [master]
branches: [master, aws-v3-support]
Copy link
Contributor

Choose a reason for hiding this comment

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

@willarmiros Can you switch this PR to draft?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good call, sorry for the spam

@willarmiros willarmiros marked this pull request as draft May 6, 2021 23:30
@willarmiros willarmiros marked this pull request as ready for review May 7, 2021 00:59
@willarmiros
Copy link
Contributor Author

@anuraaga this should now actually be ready for review if you wanna take a look :)

Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

not committing transpiled code to source control

Will you remove the current .d.ts files in another PR?

@@ -4,7 +4,7 @@ on:
push:
branches: [master]
pull_request:
branches: [master]
branches: [master, aws-v3-support]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
branches: [master, aws-v3-support]
branches: [master]

@willarmiros
Copy link
Contributor Author

Will you remove the current .d.ts files in another PR?

@anuraaga I wasn't sure whether or not I should. We have manually created .d.ts files and committed them to source control in all of our packages (not just core). I know the existing declaration files are correct, and was suspect of the generated ones. For instance, in the generated aws-xray.d.ts, where we're supposed to declare all of the top-level APIs of the SDK, this was all it contained:

export namespace middleware {
    const IncomingRequestData: typeof import("./middleware/incoming_request_data");
}

I'm sure we can make a series of changes to our source to make the generated declaration files work, but given that TS customers have been happy with the existing ones I'm not sure I'll prioritize it along with these changes.

@willarmiros
Copy link
Contributor Author

I'm going to keep CI enabled on the aws-v3-support and go ahead and merge this so I can incorporate these changes with #416. I'll disable CI and we can re-review when I make the PR for merging aws-v3-support into master.

@willarmiros willarmiros merged commit cf24754 into aws:aws-v3-support May 7, 2021
@anuraaga
Copy link
Contributor

anuraaga commented May 8, 2021

Oh ok if those are manual b declarations than sounds good didn't quite understand what's going on.

willarmiros added a commit that referenced this pull request May 12, 2021
* Revert "Revert PR #386 (#412)"

This reverts commit 9e20d2e.

* Add proper TypeScript build logic (#417)

* build core files to dist directory

* fixed unit tests

* updated workflows and readme

* try to fix windows

* fix ls

* slashes

* no more ls

* zstd decompress

* see what bin has

* removed rsync

* fixes

* added sh

* try xargs instead

* cleanups

* fixed publishing logic

* Redesign AWS SDK V3 instrumentation to use middleware (#416)

* updated deps and type file

* removed type keyword added deps

* remove changes to js

* updated versions

* finished redesign of aws sdk v3 instrumentation

* refactored buildAttributes signature

* add compile back to workflow

* bumped tsd version instead

Co-authored-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>

* removed dependencies and added docs

* add back most changes, update types

* removed aws-v3 branch from CI

* fixed codecov to use dist directory

* added blog post link

Co-authored-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants