Drupal's core dependency mck89/peast have a bug which was fixed in 1.15.2. This affects Drupal JsOptimizer which is used for JavaScript aggregation and optimization. Before that version async functions were compressed without async keyword which break aggregated JS file completely if such function rely on that (e.g. use await).
Upstream issue https://github.com/mck89/peast/issues/58
Foo = {
async bar() {},
baz: async () => {},
}
Expects to be Foo={async bar(){},baz:async()=>{}}; but Foo={bar(){},baz:async()=>{}}; provided.
bar() lost its async prefix and any await inside will lead to JS error and break aggregated JS file completely.
This was fixed in 1.15.2 version. We need to udpate dependency for Drupal core.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | 3374660-peast-11.patch | 2.41 KB | catch |
| #4 | 3374660-4.patch | 3.48 KB | niklan |
| #2 | 3374660-2-test-only.patch | 1.37 KB | niklan |
Comments
Comment #2
niklanComment #3
catchThat is probably a bug in Peast https://github.com/mck89/peast
edit: posted an upstream report https://github.com/mck89/peast/issues/58
Comment #4
niklanSeems like it was fixed and available in Peast 1.15.2 release. So I think we have to update this issue to reflect that.
Not sure if we should keep provided test, because it tests third-party library, actually.
P.s. I'm not certain about bumping minimal version, but it sounds reasonable, because otherwise it can break aggregation.
Comment #5
andypostCan you add a unit test to prevent regressions in future, that's a bug
Comment #6
chi commentedI think there is no need to create tests for third party packages. If this functionality is critical it's better to submit the test to upstream project.
Comment #7
spokjeI agree with @Chi on this one (sorry @andypost), let's see what core committers think.
Comment #8
catchPeast added their own test coverage so I think it's very unlikely to regress.
What we could maybe do if we wanted to would be to change the content of another existing test file for that test class, to include the syntax here (since they don't have any particular syntax in, just whitespace to strip) - then we'd be covering this bug without actually adding more tests to maintain overall.
I will aim to commit this early next week either way so that it definitely lands in the next patch release. Very good find and also extremely encouraging that Peast fixed it within half a day of us opening the issue.
Comment #11
spokje@catch: Could it be that this issue, due to the new non-auto-checkbox-for-credit-checking on d.o. doesn't have the credits which you would expect from reading the commit message?
I really don't need the credit, but I feel at the very least Niklan should get one for reporting the issue here, writing a fail test and commenting on the peast issue in their queue.
INSTA-EDIT: Hmm, might also be credit-less because it's not marked as fixed yet?
Comment #12
catchThe MR diff didn't cleanly apply to 11.x, I also noticed that it increased the minimum Peast version in composer.json.
So I did the update locally - Peast still updated to the latest version, but without the composer.json change.
Committed/pushed to 11.x and cherry-picked to 10.1.x, thanks!
edit: yes I was waiting for commits to push and writing the comment etc. credited everyone because the test-or-not discussion was useful for deciding that we really didn't need our own test and had thought about it enough.