removes react-iframe-comm dependency #2861
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2861 +/- ##
==========================================
+ Coverage 93.38% 93.43% +0.05%
==========================================
Files 151 152 +1
Lines 2281 2317 +36
==========================================
+ Hits 2130 2165 +35
- Misses 151 152 +1
Continue to review full report at Codecov.
|
christopher-johnson
force-pushed
the
iframecomm
branch
from
bf37054 to
d8abe04
Compare
christopher-johnson
force-pushed
the
iframecomm
branch
from
d8abe04 to
1e133b5
Compare
| @@ -0,0 +1,201 @@ | |||
| import React, { Component } from 'react'; | |||
There was a problem hiding this comment.
This is code copied from react-iframe-comm? We probably need to preserve the upstream license information in this file (and any others we're copying over.).
| /** | ||
| * | ||
| */ | ||
| export class IframeComm extends Component { |
There was a problem hiding this comment.
Is the reason for this living in src/lib because it is code copied from upstream? I would expect it to be in src/components
There was a problem hiding this comment.
lib is usually where compiled code goes so storing source in this directory is odd anyway, but in this case, since it is a "third party library component" (note also that it is a derivation, not a copy), it could/should reside in a directory that is distinct from your homegrown code (e.g. vendors) or even better, make a monorepo and build it as a separate package.
| @@ -0,0 +1 @@ | |||
| export * from './IFrameComm'; | |||
There was a problem hiding this comment.
Is this a new pattern that is needed?
| await page.waitFor(1000); | ||
| await expect(numWindows).toBe(2); | ||
| }); | ||
| }, 10000); |
There was a problem hiding this comment.
Was this needed for the replacement of the iframe component? Were the tests failing?
There was a problem hiding this comment.
no, nothing to do with the component. these puppeteer tests consistently fail for me locally and also sometimes on CI. increasing the timeout solves it.
mejackreed
left a comment
mejackreed
left a comment
There was a problem hiding this comment.
Just a few additional questions about this
|
FYI: I do not care whether this PR gets merged, but to resolve the issue, I suggest that you make whatever tangential changes you feel are appropriate. I do not spend anymore time on this. |
|
Ok thanks @christopher-johnson . We will close this for now and revisit if we need to. |
react-iframe-commbundlesreactandreact-dom. A PR towards resolving issue this upstream have had no response in several months, and it appears that this library is unmaintained. this solves the problem (and consequently reduces Mirador bundle size by ~7k). It removes the deprecated lifecyclecomponentWillReceivePropsfrom the component. Also, this resolves a console warning about the inline attribute on Typography. inWindowAuthenticationControl