New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move messaging.js to NPM package #9547
Conversation
@@ -14,7 +14,7 @@ | |||
* limitations under the License. | |||
*/ | |||
|
|||
import {Messaging, WindowPortEmulator, parseMessage} from './messaging'; | |||
import {Messaging, WindowPortEmulator, parseMessage} from 'messaging'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain how "messaging" name here is getting resolved? Also, what will this look like when imported from the "amp-viewer" repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's going to be taken straight from the file itself (not from npm)
shouldn't we be adding milestones to issues only? |
@@ -58,9 +55,10 @@ export function parseMessage(message) { | |||
if (message.charAt(0) != '{') { | |||
return null; | |||
} | |||
return /** @type {?Message} */ (tryParseJson(message) || null); | |||
return /** @type {?Message} */ (JSON.parse(/** @type {string} */ (message))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a big change - the previous version failed silently. You should probably preserve that. Anyone can send a message and it's not guaranteed to be what we expect. So it's best to shield ourselves and overspam error logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Messaging, | ||
WindowPortEmulator, | ||
parseMessage, | ||
} from '../messaging/messaging.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No ".js" in imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@chenshay Could you pls also reference the PR for |
6ac9dcc
to
9b3e477
Compare
close #9528
it is being used here: https://github.com/ampproject/amp-viewer/blob/master/src/amp-viewer-host.js