Skip to content

feat: exclude unexpected file in sourcemap result#352

Open
bvanjoi wants to merge 1 commit into
bcoe:mainfrom
bvanjoi:exclude_unexpected_file_in_result
Open

feat: exclude unexpected file in sourcemap result#352
bvanjoi wants to merge 1 commit into
bcoe:mainfrom
bvanjoi:exclude_unexpected_file_in_result

Conversation

@bvanjoi

@bvanjoi bvanjoi commented Jan 19, 2022

Copy link
Copy Markdown
Checklist
  • npm test, tests passing
  • npm run test:snap (to update the snapshot)
  • tests and/or benchmarks are included
Background

Sometimes we need to do coverage on the bundled code file, which contains a lot of useless code, for example node_modules.

Feature

This PR ensure useless files are remove by this.exclude.

@bvanjoi

bvanjoi commented Jan 19, 2022

Copy link
Copy Markdown
Author

Such as test case in my PR, which try run c8 bundle.js, and its output only contains index.js.

If we do not exclude, the result will contains index.js and node_modules/path-exists:

image

@bcoe

bcoe commented Jan 24, 2022

Copy link
Copy Markdown
Owner

@bvanjoi if I'm understanding correctly, this specifically addresses the issue of 1:many source maps, where a single bundled file might expand out into a few files, some of which are excluded?

@bvanjoi

bvanjoi commented Jan 24, 2022

Copy link
Copy Markdown
Author

@bvanjoi if I'm understanding correctly, this specifically addresses the issue of 1:many source maps, where a single bundled file might expand out into a few files, some of which are excluded?

Yeah, It is. Maybe I need to update snapshot.

@bvanjoi bvanjoi force-pushed the exclude_unexpected_file_in_result branch from 24b544a to 9b25d8e Compare January 24, 2022 14:55
@bvanjoi

bvanjoi commented Feb 6, 2022

Copy link
Copy Markdown
Author

image

Differents node version had differents branch coverage.

  • <= 12 tag branch coverage is 0.
  • but >= 14 tag it is 100.

This is probably due to the coverage file generated by node(left is node 14, and right is node12)

image

@bcoe

bcoe commented Feb 6, 2022

Copy link
Copy Markdown
Owner

@bvanjoi the culprit is the between the closing } and the catch on line 55.

> code.slice(1936, 1949)
' catch (err) '

@glromeo I don't suppose any of the work you've been doing in v8-to-istanbul happens to solve this edge-case? (I don't expect it does, since the problem is mid-line).

@glromeo

glromeo commented Feb 6, 2022

Copy link
Copy Markdown

@bcoe I checked out @bvanjoi branch and I verified that my istanbuljs/v8-to-istanbul#179 fixes that failing test.
Also istanbuljs/v8-to-istanbul#180 fixes that error but for a different reason (in general it goes one character short and picks the previous sourcemapped point if one is absent...which tends to happen for closing brackets)

@bcoe

bcoe commented Mar 23, 2022

Copy link
Copy Markdown
Owner

@bvanjoi could I bother you to update to the latest version of v8-to-istanbul on your branch, to see if it addresses some of your failing tests.

@bvanjoi

bvanjoi commented Mar 24, 2022

Copy link
Copy Markdown
Author

@bvanjoi could I bother you to update to the latest version of v8-to-istanbul on your branch, to see if it addresses some of your failing tests.

I had upgrade v8-to-istanbul to 8.1.1, but it still failed in node 12, succeed in node14, node16:

image

@bvanjoi bvanjoi force-pushed the exclude_unexpected_file_in_result branch from 44a73a0 to c56ea47 Compare March 24, 2022 14:14
@bcoe

bcoe commented Apr 20, 2022

Copy link
Copy Markdown
Owner

@bvanjoi give things a shot with the latest fixes to v8-to-istanbul?

@bvanjoi bvanjoi force-pushed the exclude_unexpected_file_in_result branch from c56ea47 to 4d03a0f Compare April 20, 2022 15:05
@bvanjoi

bvanjoi commented Apr 20, 2022

Copy link
Copy Markdown
Author

@bvanjoi give things a shot with the latest fixes to v8-to-istanbul?

It still had some problems in node 10 and node 12 😢

image

image

@bcoe

bcoe commented Apr 20, 2022

Copy link
Copy Markdown
Owner

@bvanjoi, it seems like @glromeo's upstream tweaks might be necessary for this PR to work on both Node 12 and Node 10.

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