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

chore: publish /src to npm and other fixes for sourcemap debugging #1462

Merged
merged 4 commits into from Oct 7, 2020

Conversation

justingrant
Copy link
Contributor

@justingrant justingrant commented Aug 25, 2020

This PR replaces #393. @trivikr closed that PR in January 2020 because another PR was supposed to include the same fixes, but #393 contained other fixes to problems that are still broken. Hence this PR with the remaining fixes, including updates to account for changes since Jan 2020. Content below is shamelessly copied from #393.

Currently, SDKV3 publishes sourcemaps to npm but doesn't publish the source files that those sourcemaps point to. This missing source files breaks debugging use-cases, especially for VSCode users because the VSCode debugger relies on source files for setting breakpoints in the debugger, for debugger call stacks, for "step into" original source, and any other debugger use-cases. Even outside of debugging use-cases, it's helpful for developers consuming transpiled libraries to have original source so they can better understand what the library is doing. Finally, some tools will show warnings when sourcemaps are present but source files are missing, and this PR will fix these warnings.

This PR:

  • Removes /src from .npmignore for non-client packages, for reasons discussed above.
  • Adds jest.config.js and *.spec.ts to non-client packages' .npmignore because test files shouldn't be in an NPM download.
  • Removes *.ts from npm for client packages. (also removes !*.d.ts which is now unnecessary) (This is the one part of fix: remove /src/ from .npmignore (for sourcemaps) #393 that is already fixed so is excluded from this PR)
  • In 8 non-client packages, changes sourceRoot to rootDir in tsconfig.json. The latter setting is used by all other SDKV3 packages, and it produces sourcemaps with an empty sourceRoot field. This is ideal, because the sourceRoot feature turned out to be buggy in some sourcemap-reading and sourcemap-writing tools. Leaving it blank maximizes compatibility with more tools.

This PR is similar to aws/aws-sdk-js-crypto-helpers#5 in the AWS Crypto Helpers library, and aws-amplify/amplify-js#2680 which made similar changes to AWS Amplify library earlier this year.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@justingrant justingrant changed the title Fix sourcemaps 2020 08 25 Publish /src/ to npm and other fixes for sourcemap debugging (replaces #393) Aug 25, 2020
@justingrant justingrant changed the title Publish /src/ to npm and other fixes for sourcemap debugging (replaces #393) Publish /src to npm and other fixes for sourcemap debugging (replaces #393) Aug 26, 2020
@AllanZhengYP AllanZhengYP self-requested a review September 1, 2020 16:40
@AllanZhengYP
Copy link
Contributor

Hi @justingrant, we recently merged the change that would unblock the CodeBuild test failure. Can you rebase this PR?

Crrently, SDKV3 publishes sourcemaps to npm but doesn't publish source
files that those sourcemaps point to. This missing source files breaks
debugging use-cases, especially for VSCode users because the VSCode
debugger relies on source files for setting breakpoints in the debugger,
for debugger call stacks, for "step into" original source, and any other
debugger use-cases. Even outside of debugging use-cases, it's helpful
for developers consuming transpiled libraries to have original source so
they can better understand what the library is doing. Finally, some tools
will show warnings when sourcemaps are present but source files are
missing, and this commit will fix these warnings.
@justingrant
Copy link
Contributor Author

@AllanFly120 - Rebased as requested. No merge conflicts. Thanks!

@codecov-commenter
Copy link

Codecov Report

Merging #1462 into master will decrease coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1462      +/-   ##
==========================================
- Coverage   79.80%   79.71%   -0.09%     
==========================================
  Files         298      302       +4     
  Lines       11502    11678     +176     
  Branches     2475     2489      +14     
==========================================
+ Hits         9179     9309     +130     
- Misses       2323     2369      +46     
Impacted Files Coverage Δ
...l_tests/aws-json/commands/EmptyOperationCommand.ts 90.00% <0.00%> (-10.00%) ⬇️
...ts/aws-query/commands/NoInputAndNoOutputCommand.ts 90.00% <0.00%> (-10.00%) ⬇️
.../aws-restxml/commands/NoInputAndNoOutputCommand.ts 90.00% <0.00%> (-10.00%) ⬇️
...aws-restjson/commands/NoInputAndNoOutputCommand.ts 90.00% <0.00%> (-10.00%) ⬇️
protocol_tests/aws-ec2/commands/XmlBlobsCommand.ts 95.00% <0.00%> (-5.00%) ⬇️
protocol_tests/aws-ec2/commands/XmlEnumsCommand.ts 95.00% <0.00%> (-5.00%) ⬇️
protocol_tests/aws-ec2/commands/XmlListsCommand.ts 95.00% <0.00%> (-5.00%) ⬇️
...rotocol_tests/aws-query/commands/XmlMapsCommand.ts 95.00% <0.00%> (-5.00%) ⬇️
...otocol_tests/aws-query/commands/XmlBlobsCommand.ts 95.00% <0.00%> (-5.00%) ⬇️
...otocol_tests/aws-query/commands/XmlEnumsCommand.ts 95.00% <0.00%> (-5.00%) ⬇️
... and 154 more

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 17620d6...b359ee5. Read the comment docs.

@trivikr trivikr requested review from AllanZhengYP and removed request for AllanZhengYP October 7, 2020 16:30
@alexforsyth alexforsyth changed the title Publish /src to npm and other fixes for sourcemap debugging (replaces #393) chore: publish /src to npm and other fixes for sourcemap debugging Oct 7, 2020
@codecov-io
Copy link

Codecov Report

Merging #1462 into master will decrease coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1462      +/-   ##
==========================================
- Coverage   79.80%   79.71%   -0.09%     
==========================================
  Files         298      302       +4     
  Lines       11502    11678     +176     
  Branches     2475     2489      +14     
==========================================
+ Hits         9179     9309     +130     
- Misses       2323     2369      +46     
Impacted Files Coverage Δ
...l_tests/aws-json/commands/EmptyOperationCommand.ts 90.00% <0.00%> (-10.00%) ⬇️
...ts/aws-query/commands/NoInputAndNoOutputCommand.ts 90.00% <0.00%> (-10.00%) ⬇️
.../aws-restxml/commands/NoInputAndNoOutputCommand.ts 90.00% <0.00%> (-10.00%) ⬇️
...aws-restjson/commands/NoInputAndNoOutputCommand.ts 90.00% <0.00%> (-10.00%) ⬇️
protocol_tests/aws-ec2/commands/XmlBlobsCommand.ts 95.00% <0.00%> (-5.00%) ⬇️
protocol_tests/aws-ec2/commands/XmlEnumsCommand.ts 95.00% <0.00%> (-5.00%) ⬇️
protocol_tests/aws-ec2/commands/XmlListsCommand.ts 95.00% <0.00%> (-5.00%) ⬇️
...rotocol_tests/aws-query/commands/XmlMapsCommand.ts 95.00% <0.00%> (-5.00%) ⬇️
...otocol_tests/aws-query/commands/XmlBlobsCommand.ts 95.00% <0.00%> (-5.00%) ⬇️
...otocol_tests/aws-query/commands/XmlEnumsCommand.ts 95.00% <0.00%> (-5.00%) ⬇️
... and 154 more

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 17620d6...b359ee5. Read the comment docs.

Copy link
Contributor

@AllanZhengYP AllanZhengYP left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thank you for the contribution!

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants