Bento: Coreify deserializeMessage
#35563
Conversation
|
This pull request introduces 1 alert when merging 14186a7 into 90afe34 - view on LGTM.com new alerts:
|
| }); | ||
|
|
||
| it('should return null if failed to parse the input', () => { | ||
| const errorStub = env.sandbox.stub(dev(), 'error'); | ||
| expectAsyncConsoleError(/MESSAGING: Failed to parse message/i, 2); |
There was a problem hiding this comment.
/cc @rsimha to confirm this is correct.
There was a problem hiding this comment.
The usage on line 608 looks correct, but I don't see the next couple lines doing any waiting. Should there be an await to ensure that the asynchronous error doesn't leak from the test? Or are the operations synchronous, with the errors thrown asynchronously?
There was a problem hiding this comment.
It's sync, with the error thrown async.
There was a problem hiding this comment.
Thanks for clarifying. In that case, this code should work as written. Any issues will be caught by the "Local Unit Tests" job on CircleCI.
There was a problem hiding this comment.
It looks like the test is failing due to the error: https://app.circleci.com/pipelines/github/ampproject/amphtml/14320/workflows/8164eb80-8a86-4022-9fe8-6edb28601c2c/jobs/239541
This case expects that the function will return null even if given malformed json, which should cause the async error. Any tips on how to debug?
Co-authored-by: Justin Ridgewell <justin@ridgewell.name>
It can be moved but I wasn't sure if it is urgent to do so given that the larger file may be moved eventually. Is it generally preferred for (1) files to import either from |
If I'm reading the imports correctly, this entire file should be safe/ready to import into |
Sounds good! I'll follow up with this. Thanks. |
src/3p-frame-messaging.js
Outdated
| import {internalListenImplementation} from './core/dom/event-helper-listen'; | ||
| import {rethrowAsync} from './core/error'; |
There was a problem hiding this comment.
nit: Let's update all of these to use #core instead of ./core. It'll make moving the file a no-diff operation later. Really all files using this notation should be updated, but it hasn't been prioritized yet
There was a problem hiding this comment.
Good idea, I was wondering about that when my auto import for rethrowAsync put it in #core but the others were in ./core. Updated :)
* Update src/*.js to import via alias * Revert 3p-frame-messaging (updated in #35563) * Fix rebase error
* Integrate expectAsyncConsoleError with rethrowAsync Re: #35563 * Cleanup
) * Integrate expectAsyncConsoleError with rethrowAsync Re: ampproject#35563 * Cleanup
The helper is used in a number of Bento components including
amp-twitter,amp-facebook, andamp-embedly-card.