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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: replace comparison #4321

Closed
wants to merge 2 commits into from
Closed

Conversation

dnalborczyk
Copy link
Contributor

@dnalborczyk dnalborczyk commented Dec 27, 2021

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

@lukastaegert jus a follow-up from: #4302 (comment).

let me know if you have any questions. I can explain it a bit more (it's a bit late right now 馃槃 ) I'm trying to achieve that the types can eventually be trusted. 馃槈 although, someone might have to start likely with acorn to achieve that: acornjs/acorn#946

@codecov
Copy link

codecov bot commented Dec 27, 2021

Codecov Report

Merging #4321 (665a837) into master (2a899d5) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4321   +/-   ##
=======================================
  Coverage   98.44%   98.44%           
=======================================
  Files         205      205           
  Lines        7314     7314           
  Branches     2084     2084           
=======================================
  Hits         7200     7200           
  Misses         55       55           
  Partials       59       59           
Impacted Files Coverage 螖
src/utils/getOriginalLocation.ts 82.35% <100.00%> (酶)

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 2a899d5...665a837. Read the comment docs.

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

I am sorry but I do not see how this improves anything. The types have been working before, the previous check for truthiness was correct as in all cases where .mappings was falsy, it was definitely not a valid SourceMap. So why are you so determine to add this? Sure, the TypeScript types say it should be enough to compare against undefined, but in the end, source maps are generated by plugins and may have been generated completely without any types. TypeScript cannot give any guarantees here. And at this point, maybe some build setup was relying on this, who knows. The more general check we had was basically for free, so I would prefer to keep it.

@dnalborczyk dnalborczyk deleted the get-orig-loc branch May 19, 2023 02:08
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.

None yet

2 participants