fix(deps): switch from @iarna/toml to smol-toml#6837
fix(deps): switch from @iarna/toml to smol-toml#6837dario-piotrowicz wants to merge 4 commits intonetlify:mainfrom
@iarna/toml to smol-toml#6837Conversation
|
PS: for a bit of extra context, I am proposing this change since in wrangler, in order to avoid the eval warnings at build, we are patching the build-info library (cloudflare/workers-sdk#11452), having the patch will be bothersome for to keep up with the library's changes (as we'll have to keep updating the patch as well), so that's the main reason why I wanted to propose the change Also I figured this change could be applied to all packages in this monorepo, but if you think this is too risky I'm happy to also restrict this to only the build-info package. |
@iarna/toml to smol-toml@iarna/toml to smol-toml
@iarna/toml to smol-toml@iarna/toml to smol-toml
serhalp
left a comment
There was a problem hiding this comment.
LGTM. This is great, thank you! We have a lot of deps we'd like to clean up in here, so thanks for knocking this one out 😄.
FYI: I'll release this separately from the new framework PRs. I want to be a bit careful with this one as it touches a critical path and there's always a chance of some subtle TOML handling behavioural edge cases that aren't covered by tests.
| Unterminated string at row 1, col 3, pos 2:␊ | ||
| 1> "␊ | ||
| ^␊ | ||
| 2: ␊ | ||
| Invalid TOML document: incomplete key-value: cannot find end of key␊ | ||
| ␊ | ||
| 1: "␊ | ||
| ^␊ |
There was a problem hiding this comment.
👍🏼 Seems fine. I wouldn't consider the exact sub-error message that we print for malformed netlify.toml files to be part of the stable public interface of the package.
Summary
@iarna/tomldoesn't seem to be actively maintained (see: https://github.com/iarna/iarna-toml/graphs/contributors)(its latest version being published 6 years ago: https://www.npmjs.com/package/@iarna/toml?activeTab=readme).
It also includes some
evalcalls that are not generally recommended and cause bundling tools such as esbuild to show warnings during build.So for these reasons I am proposing to switch to
smol-tomlinstead. The library seems to be actively maintained (https://github.com/squirrelchat/smol-toml/graphs/contributors) (its latest release being about month old: https://www.npmjs.com/package/smol-toml).According to their readme (https://github.com/squirrelchat/smol-toml?tab=readme-ov-file#performance) the package should also be significantly more performant than
@iarna/toml.Surprisingly not many code changes were needed here, but given that the tests pass I am assuming that this is enough 🤔
For us to review and ship your PR efficiently, please perform the following steps:
we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or
something that`s on fire 🔥 (e.g. incident related), you can skip this step.
your code follows our style guide and passes our tests.
A picture of a cute animal (not mandatory, but encouraged)