Skip to content

Conversation

ArrayZoneYour
Copy link
Contributor

@ArrayZoneYour ArrayZoneYour commented Jul 17, 2022

Reference:

https://github.com/istanbuljs/nyc#source-map-support-for-pre-instrumented-codebases

#293

Checklist
  • documentation is changed or added

Copy link
Owner

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution 👏

I left some suggestions about how we might better explain the difference between inline vs., on disk source maps, and the setting --exclude-after-remap.

README.md Outdated
transpiler like [`@babel/register`]) c8 supports both inline source-maps and
`.map` files.

_Important: If you are using c8 with a project that pre-instruments its code,
Copy link
Owner

Choose a reason for hiding this comment

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

It would be useful to give an example of when we would want to set --exclude-after-remap to true, vs, when we would want to leave it false, rather than this blurb.

README.md Outdated
and `--exclude` flag checks, will be loaded into the report. If any of those files remain uncovered they will be factored
into the report with a default of 0% coverage.

## Source-Map support for pre-instrumented codebases
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps instead:

Pre-instrumented vs., just-in-time instrumented codebases

Source map files, vs., inline source maps

Just-in-time instrumented codebases will often insert source maps inline with the .js code they generate at runtime (_as an example, @babel-register/register can be configured to insert a source map foote).

Pre-instrumented codebases, e.g., running tsc to generate .js in a build folder, may generate either inline source maps, or a separate .map file stored on disk.

c8 can handle loading both types of source maps.

Exclude after remap

... explain here why you when you would set the setting to true, vs., false.

@ArrayZoneYour
Copy link
Contributor Author

@bcoe Thanks for suggestions and review 😀. Update the explanations and be willing to make some other changes if it is required.

Copy link
Owner

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

Left a couple more editing suggestions, does this get at what you were picturing?

ArrayZoneYour and others added 2 commits July 20, 2022 00:01
Co-authored-by: Benjamin E. Coe <[email protected]>
Co-authored-by: Benjamin E. Coe <[email protected]>
@ArrayZoneYour ArrayZoneYour requested a review from bcoe July 19, 2022 16:05
@bcoe bcoe merged commit ac99234 into bcoe:main Jul 19, 2022
@bcoe
Copy link
Owner

bcoe commented Jul 19, 2022

@ArrayZoneYour thank you for the contribution.

mcknasty pushed a commit to mcknasty/c8 that referenced this pull request Feb 2, 2024
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.

2 participants