Skip to content

prevent crash in locStart/locEnd when node location is undefined #864

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Conversation

stonerl
Copy link

@stonerl stonerl commented Apr 11, 2025

Prettier's formatWithCursor can crash if a node is missing a valid location object.

This patch adds null-safe checks and fallback handling in locStart and locEnd to avoid TypeErrors and ensure graceful degradation.

Prettier's formatWithCursor can crash if a node is missing a valid `location` object.

This patch adds null-safe checks and fallback handling in `locStart` and `locEnd`
to avoid TypeErrors and ensure graceful degradation.

Signed-off-by: Toni Förster <toni.foerster@icloud.com>
@kddnewton
Copy link
Member

Can you give me a text XML file where this happens? I have not found an example. If there is a test you could add, that would be great. I don't want to be needlessly defensive here.

@stonerl
Copy link
Author

stonerl commented Apr 12, 2025

@kddnewton no worries. It happens with this test file, with my Prettier extension for Nova.

The error message I get can be found in the issue in the first comment: stonerl/nova-prettier-extension#41 (comment)

@stonerl
Copy link
Author

stonerl commented Apr 12, 2025

It could also be me implementing formatWithCursor incorrect, but I only get this error with the xml-plugin. Everything else works.

@kddnewton
Copy link
Member

I don't think you're doing it wrong, but is it possible you're using an earlier version of the plugin? I cannot get it to crash with the latest version.

@stonerl
Copy link
Author

stonerl commented Apr 12, 2025

I'm using the 3.4.1 that's the latest one AFAIK.

I currently apply this patch when installing all npm packages, and it circumvents the error and allows formatting XML files.

This could be something that happens only in combination with Nova. I don't mind if this patch won't go into upstream, I can keep it as part of my extension.

@stonerl
Copy link
Author

stonerl commented Apr 12, 2025

I think I might know what's going on here. Nova is using Apple's JavaScript Core engine.

I assume you're using node.js for testing? It could be that this is just a platform specific error.

@kddnewton
Copy link
Member

I don't think so, if I run this in bun or safari it passes. I would be shocked if this had anything to do with the runtime as it's a logic error. Are you 100% sure you're on the latest version?

@stonerl
Copy link
Author

stonerl commented Apr 13, 2025

Pretty sure:

nova-prettier-extension/prettier.novaextension on  feature/config-file [!] is 📦 3.5.16 via ⬢ v23.11.0
➜ npm list
nova-prettier-extension@3.5.16 /Users/toni/Developer/nova-prettier-extension/prettier.novaextension
├── @prettier/plugin-php@0.22.4
├── @prettier/plugin-xml@3.4.1
├── patch-package@8.0.0
├── prettier-plugin-java@2.6.7
├── prettier-plugin-nginx@1.0.3
├── prettier-plugin-properties@0.3.0
├── prettier-plugin-sql@0.18.1
└── prettier@3.5.3

@stonerl
Copy link
Author

stonerl commented Apr 23, 2025

Just an update. I ran into the same error while implementing pretter-plugin-astro and prettier-plugin-sh and had to fix it the same way.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants