-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
feat: Introduce a library for embedded iframe <-> host communication #18652
Conversation
@@ -22,7 +22,7 @@ | |||
"module": "lib/index.js", | |||
"types": "dist/index.d.ts", | |||
"scripts": { | |||
"build": "tsc & babel src --out-dir lib --extensions '.ts,.tsx' & webpack --mode production", | |||
"build": "tsc ; babel src --out-dir lib --extensions '.ts,.tsx' ; webpack --mode production", |
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.
I didn't realize this before but &
actually starts a process in the background instead of working like Promise.all
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.
"build": "tsc ; babel src --out-dir lib --extensions '.ts,.tsx' ; webpack --mode production", | |
"build": "tsc && babel src --out-dir lib --extensions '.ts,.tsx' && webpack --mode production", |
I suggest use &&
to execute that in series only if the predecessor was successful. In other words, stop as soon as an error occurs.
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.
These aren't really dependent on each other's result. Running each of them regardless of the others' status can output more useful information in the case of an error. Ideally I'd like to run them in parallel and wait for all to complete before the command exits, but I am not familiar with a convenient way to do that without writing a script, and that seems like overkill since these all run in about a second.
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.
LGTM. please fix lint before merge :)
Codecov Report
@@ Coverage Diff @@
## master #18652 +/- ##
==========================================
- Coverage 66.30% 66.22% -0.09%
==========================================
Files 1595 1605 +10
Lines 62629 62797 +168
Branches 6308 6341 +33
==========================================
+ Hits 41527 41585 +58
- Misses 19453 19560 +107
- Partials 1649 1652 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
SUMMARY
This library is pretty simple, the code comments should explain everything.
This also uses the new library to provide a method to the sdk.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION