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

chore: backwards compat for dash-case warnings #11495

Closed
wants to merge 7 commits into from

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented May 6, 2024

Compiler warnings that are dashed are also taken into account for ignoring warnings for backwards compatiblity (else many apps would be littered with a11y warnings now).
Also adds logic to the migration script to convert these to underscores.
Along the way I discovered that attribute a11y warnings were unsilenceable, the path upwards traversal was more robust in that regard.

Related to #11414
fixes #11482

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

Compiler warnings that are dashed are also taken into account for ignoring warnings for backwards compatiblity (else many apps would be littered with a11y warnings now).
Also adds logic to the migration script to convert these to underscores.
Copy link

changeset-bot bot commented May 6, 2024

🦋 Changeset detected

Latest commit: 546cc7d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Rich-Harris
Copy link
Member

I think we should probably try and remap the warnings that have changed beyond just _ -> -, especially for the purposes of the migration script — I don't think there's that many. I've been meaning to get round to this (should probably have taken notes while I was doing the overhaul, but oh well...)

I also think we should only do this in non-runes mode. In runes mode we should warn on any unknown codes.

@dummdidumm
Copy link
Member Author

dummdidumm commented May 8, 2024

  • only ignoring the old warning codes in legacy mode now
  • adjusted the ignore logic while I was in that area (using the same global context approach that is already used for warnings)
  • started a map of new codes -> old codes (and vice versa). Feel free to add more codes you know you have renamed - but don't let perfect be the enemy of the good, I think it's more useful to merge this now and add those when we come across them. People have their console littered with warnings right now and it would be good to get this out quickly

"column": 29,
"line": 5
},
"message": "`<img>` element should have an alt attribute (this warning was tried to silence using code a11y-missing-attribute. In runes mode, use the new code a11y_missing_attribute instead)",
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be simpler and more helpful to warn on <!-- svelte-ignore a11y-missing-attribute --> in runes mode, and leave the other warning untouched

Copy link
Member

Choose a reason for hiding this comment

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

(working on this)

@dummdidumm
Copy link
Member Author

Closing in favor of the other PRs that did this.

@dummdidumm dummdidumm closed this May 14, 2024
@dummdidumm dummdidumm deleted the dash-case-warnings branch May 14, 2024 09:36
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.

Svelte 5: behavior of svelte-ignore is different between Svelte 4 and Svelte 5
2 participants