Skip to content

fix: comment stripping should remove multiline comments (#279)#280

Open
cjpearson wants to merge 2 commits into
unjs:mainfrom
cjpearson:feature/comment-stripping
Open

fix: comment stripping should remove multiline comments (#279)#280
cjpearson wants to merge 2 commits into
unjs:mainfrom
cjpearson:feature/comment-stripping

Conversation

@cjpearson

Copy link
Copy Markdown

Resolves #279

@pi0

pi0 commented Oct 6, 2024

Copy link
Copy Markdown
Member

Thanks for the PR dear @cjpearson

Removing multi-line comments without AST awareness can be unsafe this is why we don't it.

See #56 for some context.

Until #219 arrives, if we can find similar solution to #56 i am open to it otherwise i would wait.

@codecov

codecov Bot commented Oct 6, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.08%. Comparing base (226a47b) to head (c4ab502).
Report is 62 commits behind head on main.

Files with missing lines Patch % Lines
src/syntax.ts 91.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #280      +/-   ##
==========================================
- Coverage   88.04%   84.08%   -3.96%     
==========================================
  Files           8        8              
  Lines        1062      823     -239     
  Branches      188      201      +13     
==========================================
- Hits          935      692     -243     
- Misses        127      131       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cjpearson

Copy link
Copy Markdown
Author

Oh I didn't realize acorn was already in this package. I think that would be a much better way to handle comments. I'll take a look at that approach.

@cjpearson cjpearson force-pushed the feature/comment-stripping branch from b2cf9ff to 176fb90 Compare October 7, 2024 11:31
@cjpearson

Copy link
Copy Markdown
Author

I've tweaked the approach to use acorn rather than regular expressions for the comment stripping. My understanding is #219 will cover replacing ESM_RE and CJS_RE, so I made it so the change just affects comment stripping portion. Once #219 is done I suppose the comment stripping will be entirely unnecessary

@cjpearson cjpearson force-pushed the feature/comment-stripping branch from 176fb90 to c4ab502 Compare October 7, 2024 11:35
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.

Multiline comments are not stripped

2 participants