Skip to content

in case properties is undefined #110

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

Closed
wants to merge 1 commit into from

Conversation

xiaoxinghu
Copy link

properties field in Element type is optional (according to hast definition). when they are, this will crash. Maybe should be more tolerate here?

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and discussions and couldn’t find anything or linked relevant results below
  • I made sure the docs are up to date
  • I included tests (or that’s not needed)

Description of changes

use optional chaining when accessing properties of Element.

properties field in Element type is optional (according to hast definition). when they are, this will crash. Maybe should be more tolerate here?
@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Feb 16, 2025
Copy link

codecov bot commented Feb 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (e99b9d0) to head (30b8491).
Report is 20 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #110   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        14    -1     
  Lines         1823      1831    +8     
=========================================
+ Hits          1823      1831    +8     

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

@wooorm
Copy link
Member

wooorm commented Feb 17, 2025

Hi! :)

according to hast definition

I do not think so. Where do you base this statement on?


Sounds like you are using/writing a plugin with a bug. Better solve it there!

@wooorm wooorm closed this Feb 17, 2025
@wooorm wooorm added the 👀 no/external This makes more sense somewhere else label Feb 17, 2025

This comment has been minimized.

@github-actions github-actions bot added 👎 phase/no Post cannot or will not be acted on and removed 🤞 phase/open Post is being triaged manually labels Feb 17, 2025
@JounQin
Copy link
Member

JounQin commented Feb 17, 2025

@wooorm
Copy link
Member

wooorm commented Feb 17, 2025

Huh! Not sure where that ? is from. It wasn’t there earlier: https://github.com/syntax-tree/hast/blob/845fa62a70597cb50b80bbe76e33067169d4fd68/readme.md#element. I don’t remember intentionally adding optionality.

wooorm added a commit to syntax-tree/hast that referenced this pull request Feb 17, 2025
@JounQin
Copy link
Member

JounQin commented Feb 17, 2025

I don’t remember intentionally adding optionality.

😄 7 years ago.

syntax-tree/hast@3d15ab3#diff-5a831ea67cf5cf8703b0de46901ab25bd191f56b320053be9332d9a3b0d01d15R113-R118

@xiaoxinghu
Copy link
Author

Nice! Thanks for the clarification.

@xiaoxinghu xiaoxinghu deleted the patch-1 branch February 18, 2025 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 no/external This makes more sense somewhere else 👎 phase/no Post cannot or will not be acted on
Development

Successfully merging this pull request may close these issues.

3 participants