fix: better assetsInlineLimit runtime type checking

[Cherry]

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#L419

Testing

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.

[changeset-bot]

🦋 Changeset detected

Latest 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

[bluwy]

But assetsInlineLimit doesn't accept a string? I think it only happened to work (https://vitejs.dev/config/build-options.html#build-assetsinlinelimit).

[Cherry]

While true, it's documented to only accept a number, passing '0' especially has been used and is even still recommended every now and then in Astro's discord.

• [Build] vanilla js is not putted into separeted file like css #5083 (comment)
• https://discord.com/channels/830184174198718474/1197000366752026645/1197000809318203424
• https://discord.com/channels/830184174198718474/1195099820839407707/1195099822911389818

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?

[bluwy]

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.

[Cherry]

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 '0' recommended to them.

If the error was clearer perhaps and explicitly called out "assetsInlineLimit was passed a string of 0. This isn't supported - please use a number instead" then I could understand that perspective, but when it fails with an internal error of the following, I don't think that's a great developer experience:

[astro:rollup-plugin-inline-stylesheets] assetsInlineLimit is not a function
  Stack trace:
    at shouldInlineAsset (file:///E:/GitHub/astro-testing/verdant-visual/node_modules/astro/dist/core/build/plugins/util.js:44:18)
    at Array.forEach (<anonymous>)
    at file:///E:/GitHub/astro-testing/verdant-visual/node_modules/rollup/dist/es/shared/node-entry.js:19579:40

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/

[matthewp]

How many reports of this breaking peoples apps have we gotten? I would agree to take some action if it were a large number.

[Cherry]

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 null or some other data-type would also error in 4.4.x since the code now assumes anything that's not a number is a function), I figure it would be a good idea to merge this and fix the edge-cases, as well as improve DX.

[Cherry]

If explicitly supporting a string isn't something that's wanted to be supported, would you accept the typeof function check at least, as like in Vite, to improve the DX people get when passing something they shouldn't?

[matthewp]

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 Number(assetsInlineLimit) without checking the type? If it's NaN it will fail the check, everything should be good, right?

[Cherry]

Thanks @matthewp!

Can't we just do Number(assetsInlineLimit) without checking the type?

This was effectively my initial fix for this, and matches the Vite behaviour most closely: 8acd6a3 (#10154)

I refactored a little bit after the review here to check for NaN, and added the typeof function check to also better match Vite's behaviour: https://github.com/vitejs/vite/blob/71dc6a6b7d41c27133f04b92256bead74b8f2127/packages/vite/src/node/plugins/asset.ts#L414. The NaN check does make sense for if someone passes abc for example, but I understand that's definitely very niche and edge-casey.

I'm very happy to simplify this again if requested!

[Princesseuh]

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

[bluwy]

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.

[Cherry]

Thanks for working through this, all. I'm happy to simplify and remove the NaN check if that's the consensus, as well as the tests if needed - let me know.

[matthewp]

@Cherry Yes, agree with @bluwy's compromise here. Thanks for working with us!

[Cherry]

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.

[bluwy]

Thanks for making the change!