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

feat: exclude unexpected file in sourcemap result #352

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bvanjoi
Copy link

@bvanjoi bvanjoi commented Jan 19, 2022

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
Copy link
Author

bvanjoi commented Jan 19, 2022

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
Copy link
Owner

bcoe commented Jan 24, 2022

@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
Copy link
Author

bvanjoi commented Jan 24, 2022

@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
Copy link
Author

bvanjoi commented Feb 6, 2022

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
Copy link
Owner

bcoe commented Feb 6, 2022

@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
Copy link

glromeo commented Feb 6, 2022

@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
Copy link
Owner

bcoe commented Mar 23, 2022

@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
Copy link
Author

bvanjoi commented Mar 24, 2022

@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
Copy link
Owner

bcoe commented Apr 20, 2022

@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
Copy link
Author

bvanjoi commented Apr 20, 2022

@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
Copy link
Owner

bcoe commented Apr 20, 2022

@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