-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
fix: better assetsInlineLimit runtime type checking #10154
Conversation
🦋 Changeset detectedLatest commit: d721ccc The changes in this PR will be included in the next version bump. 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 |
But |
While true, it's documented to only accept a number, passing
As well as a handful of projects using it around GitHub: https://github.com/search?q=%22assetsInlineLimit%3A+%5C%22%22+OR+%22assetsInlineLimit%3A+%27%22&type=code etc. Considering how it worked in 4.3.x, but then stopped working and broke projects in 4.4.x, I think it would be good to handle this at least in 4.x, and officially dropped in 5.x, perhaps with the next major version of Vite? |
I don't think it being misused or misrecommended today means that it is a stable API that we can't break. We had always documented it to accept numbers, so Vite should be able to fix that in a patch even. Hence I don't think we should allow strings here. |
I truly think that when it's being recommended in the official Astro discord server, some consideration needs to be taken here not to break existing projects, especially when folks search for docs around this subject and find the solution of If the error was clearer perhaps and explicitly called out "
Astro's 4.4 release doesn't call out any breaking changes, or any real changes in behaviour, so when folks go to upgrade and hit this obscure error (like I did), they'll be left confused until they go digging into the source and their config to discover what might have changed. Vite's implementation at least checks if this is a function first before attempting to call it. I would urge you to reconsider your perspective here. This feels like a case of Hyrum's Law to me: https://www.hyrumslaw.com/ |
How many reports of this breaking peoples apps have we gotten? I would agree to take some action if it were a large number. |
With Astro 4.4 releasing on the tail end of last week, I believe I'm the first to officially hit and report this. But with the AI bot in Discord still recommending this as recently as just 2 days ago in the Discord, I won't be the only one. For the small addition this adds to the codebase, with much better runtime type-guarding (passing |
If explicitly supporting a |
Thanks for arguing your position with passion @Cherry! One thing I'm not understanding is, why is so much code needed to support strings in the first place? Can't we just do |
Thanks @matthewp!
This was effectively my initial fix for this, and matches the Vite behaviour most closely: I refactored a little bit after the review here to check for I'm very happy to simplify this again if requested! |
The only thing I'm getting from this thread is that AI chat bots were a mistake. I understand your point completely, and it's ultimately neither your or our fault really, but I cannot help but be extremely annoyed at the fact that we're accommodating for an inaccurate technology 🤦♀️ But I guess in the first place we're accommodating for JavaScript not having types, everything sucks! Your fix looks fine to me |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If others are fine with this change, I won't block this though. However, I'd prefer if we check as typeof
function only and unconditionally coerce it with Number(...)
like Vite. (so no NaN
checks). I'd also update the changeset to mention that its not a regression and remove the tests as I don't think it's something we should guarantee not to break again.
While we're indeed using JS and types are only a first-line of defence, it would be taxing to have to validate every option exposed on the config so that everything works as expected. Hence, types helps draw that line to prevent perf implications.
Thanks for working through this, all. I'm happy to simplify and remove the |
Thanks folks, I've updated to simplify the check now, and tweaked the changeset. Let me know if there's anything further you'd like to see. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the change!
Changes
Fixes #10153
This now mimics the behaviour in Vite core directly, which passes the config option to
Number
first: https://github.com/vitejs/vite/blob/71dc6a6b7d41c27133f04b92256bead74b8f2127/packages/vite/src/node/plugins/asset.ts#L419Testing
I added a test to the
css-assets
test file, which was essentially a copy/paste of the existing one there, but sets the config option to a string instead of a number. Before my changes, this was failing, but now it's passing as per previous behaviour in 4.3.x.If there's a better place to add a test for this, please let me know.
Docs
I don't think any additional docs are needed here, as it's just a minor regression from 4.3.x.