fix: functions with name matching config prop cause crash #4748
Conversation
📊 Benchmark resultsComparing with 74527f4 Package size: 227 MB(no change) Legend
|
| @@ -840,4 +840,25 @@ test('should handle content-types with charset', async (t) => { | |||
| }) | |||
| }) | |||
|
|
|||
| test('should execute function when name clashes with config key names', async (t) => { | |||
There was a problem hiding this comment.
The fix itself looks good to me, but I'm not sure this is the right test to add. The fact that a function name was clashing with a config property was not the underlying problem, it was one of the side effects. The underlying problem was that CLI was sending the wrong configuration object to zip-it-and-ship-it, and I think that's what we should assert in the test.
I think the best way to do that would be to stub the call to zip-it-and-ship-it, which isn't very straightforward in this type of integration test (i.e. using withDevServer and testing the end-to-end flow).
Instead, you could write a unit test for the functions registry, asserting that it receives the entire configuration in its constructor but then passes just the functions block to zip-it-and-ship-it.
There was a problem hiding this comment.
I disagree slightly here. Having a unit test is the way to go, but I would keep the integration test. Even though it might not have been the root cause, it is a really good test to have.
If we ever have the same wrong behavior again due to refactors or other changes, but because of a different root cause we would catch it here.
| @@ -840,4 +840,25 @@ test('should handle content-types with charset', async (t) => { | |||
| }) | |||
| }) | |||
|
|
|||
| test('should execute function when name clashes with config key names', async (t) => { | |||
There was a problem hiding this comment.
I disagree slightly here. Having a unit test is the way to go, but I would keep the integration test. Even though it might not have been the root cause, it is a really good test to have.
If we ever have the same wrong behavior again due to refactors or other changes, but because of a different root cause we would catch it here.
Co-authored-by: Daniel Tschinder <[email protected]>
📊 Benchmark resultsComparing with 13dd3a3 Package size: 227 MB(no change) |
JWhist
dismissed
eduardoboucas’s stale review
Eduardo is OOO, and review is stale
|
Unable to merge this due to problems with the CI failing (integration test 130 in particular, and other eleventy tests unrelated to this change, they were fine yesterday - I've retried numerous times) |
🎉 Thanks for submitting a pull request! 🎉
Summary
Fixes #4679
Fixes bug by sending appropriate functions config data to zisi for bundling.
For us to review and ship your PR efficiently, please perform the following steps:
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)