Create a plugin component to override file previews #2519

chetanyakan · 2019-03-19T13:41:09Z

Summary

Add a plugin component to override file previews.

Ticket Link

Checklist

Ran make check-style to check for style errors (required for all pull requests)
Ran make test to ensure unit and component tests passed
Added or updated unit tests (required for all new features)
Touches critical sections of the codebase (file preview)

saturninoabril · 2019-03-20T14:58:44Z

Thanks @hanzei but I'll defer review to @levb in this case since he's given more thoughts on the direction of this issue.

(updated)

hanzei · 2019-03-20T15:00:20Z

My idea was to split the reviews among teams. But 1/5.

levb

On the surface, looks good to me other than a couple naming suggestions.

I lack React expertise to assert the correctness, will leave it to @crspeller

components/view_image/view_image.jsx

plugins/registry.js

chetanyakan · 2019-03-22T08:54:51Z

@levb @crspeller There is another change in this PR. I have exposed PDFJS to the window so that we can use it from plugins as Webpack externals. https://github.com/mattermost/mattermost-webapp/pull/2519/files#diff-b6fe17723dee9e7445e3645b93f535caR15

Also, I think this is good for re-review.

jasonblais · 2019-03-22T16:13:41Z

Given Lev is out, have requested Joram's help with review.

jwilander · 2019-03-22T16:19:24Z

components/view_image/view_image.jsx

@@ -262,6 +269,18 @@ export default class ViewImageModal extends React.PureComponent {
 );
 }

+ for (const preview of this.props.pluginFilePreviewComponents) {
+ if (preview.override(fileInfo, this.props.post)) {


What happens if I have multiple plugins trying to override file previews and two of the plugins want to override the same preview? The only other case we have overrides like this is for post components based on the post's type which we provide the best practice of only overriding types specific to your plugin to prevent your plugin negatively affecting other plugins. Is there a best practice we can offer here to avoid two different plugins colliding here (e.g one getting clobbered by the other)? If not, we should try to come up with a way to do this that will allow multiple plugins using this feature to work well together

So, the idea here is that preview component of the first plugin that can override a preview for the given fileInfo and post objects will be used.

This is in a sense similar to the custom post type component, where if multiple plugins try to register the same post-type, the first plugin component in sorted order of plugin-ids is used.

Although this may change depending on plugin requirements, one best practice we could suggest plugin authors is to override file previews for specific file types. An example would be that one plugin would override previews for pdf files and another for office files.

Not sure there is a lot that can be done here. I hesitate to make this more complicated than it needs to be just to allow multiple plugins to coexist. The best practice suggested that plugins limit themselves to one or a reasonably small set of related extensions seems reasonable.

The only case I worry about here is some kind of "better previews for MM" plugin clashing with "super png previews for MM" plugin. Where I want the second to override the first. The only way I see resolving that is somehow allowing the user to specify which one gets priority. A workaround could be to rename the second asuperpngpreivews because then it gets sorted first.

Anyway I think that this might be out of scope for this PR.

Agreed that there probably isn't a great solution to this. I'm fine with it as is then but we should clearly document the dangers of overriding all file previews. A comment on the register API saying something like:

// Only one plugin can override a file preview at a time. If two plugins try to override the same file preview, the first plugin will perform the override and the second will not. Plugin precedence is ordered alphabetically by plugin ID.

jwilander · 2019-03-22T16:25:12Z

components/single_image_view/single_image_view.jsx

@@ -18,7 +18,7 @@ const PREVIEW_IMAGE_MIN_DIMENSION = 50;

 export default class SingleImageView extends React.PureComponent {
 static propTypes = {
- postId: PropTypes.string.isRequired,
+ post: PropTypes.object.isRequired,


Instead of updating this to receive the post object as a prop, can you instead continue passing the post ID and then just pull the correct post using a redux selector in the view_image/index.js? It's generally better to avoid passing objects down multiple layers as props where avoidable and to pass primitives instead

Makes sense. Updated.

- Update test cases

… MI-373

crspeller · 2019-03-25T05:26:44Z

components/view_image/view_image.jsx

@@ -262,6 +269,18 @@ export default class ViewImageModal extends React.PureComponent {
 );
 }

+ for (const preview of this.props.pluginFilePreviewComponents) {
+ if (preview.override(fileInfo, this.props.post)) {


Not sure there is a lot that can be done here. I hesitate to make this more complicated than it needs to be just to allow multiple plugins to coexist. The best practice suggested that plugins limit themselves to one or a reasonably small set of related extensions seems reasonable.

The only case I worry about here is some kind of "better previews for MM" plugin clashing with "super png previews for MM" plugin. Where I want the second to override the first. The only way I see resolving that is somehow allowing the user to specify which one gets priority. A workaround could be to rename the second asuperpngpreivews because then it gets sorted first.

Anyway I think that this might be out of scope for this PR.

crspeller · 2019-03-25T05:31:14Z

plugins/export.js

+window.PropTypes = require('prop-types');
+window.PDFJS = require('pdfjs-dist');


I think this is only required for certain libraries because they need to be consistent with the webapp or there can only be one loaded on the window at a time. I don't think pdfjs falls into this category.

What's your reasoning for exporting this?

If pdfjs-dist is installed via npm and imported in a plugin component, the following warning is shown while building the plugin:

WARNING in asset size limit: The following asset(s) exceed the recommended size limit (244 KiB). This can impact web performance. Assets: main.js (437 KiB) 1.main.js (698 KiB) WARNING in entrypoint size limit: The following entrypoint(s) combined asset size exceeds the recommended limit (244 KiB). This can impact web performance. Entrypoints: main (437 KiB) main.js

By exposing PDFJS using Webpack externals, we can avoid the above warning.

I was running into several issues with using PDFJS in a plugin without the above change.
By exposing the PDFJS object to the window, it would be easier for plugin developers to render PDFs.

Yes, I guess it's a bit large, maybe worth it to export.

What where those issues?

2.> Uncaught TypeError: r(...) is not a function would be logged to the console.

You can go to the current master branch and see this if you run npm i -S pdfjs-dist and add import PDFJS from 'pdfjs-dist'; in any plugin.
I have tried with different versions of PDFJS. Couldn't find much help from the internet about this issue as well.

But this does get resolved by eporting PDFJS to the window from mattermost-webapp to be used in plugins.

Alright I think this is fine then.

amyblais · 2019-03-26T17:55:11Z

@crspeller Can you help review this PR today? This will need to make it for today's Feature Complete.

plugins/registry.js

crspeller · 2019-03-26T20:06:07Z

@chetanyakan Just need to accept the documentation suggested change, then I think this PR is good to go.

Lev is out.

Co-Authored-By: chetanyakan <[email protected]>

chetanyakan · 2019-03-27T04:20:14Z

@crspeller Done.

* Create a plugin component to override file previews * Add test cases * Code refactoring * Expost pdfjs to window to use in webpack externals in plugins * - Pass postId to ViewImageModal and use getPost to fetch post object. - Update test cases * Update plugins/registry.js Co-Authored-By: chetanyakan <[email protected]>

Create a plugin component to override file previews

3e4bfb1

chetanyakan changed the title ~~Create a plugin component to override file previews~~ WIP: Create a plugin component to override file previews Mar 19, 2019

jasonblais added the Work in Progress Not yet ready for review label Mar 20, 2019

jasonblais added this to the v5.10.0 milestone Mar 20, 2019

Add test cases

443d311

chetanyakan changed the title ~~WIP: Create a plugin component to override file previews~~ Create a plugin component to override file previews Mar 20, 2019

hanzei requested review from saturninoabril and crspeller March 20, 2019 11:40

hanzei added 2: Dev Review Requires review by a core commiter and removed Work in Progress Not yet ready for review labels Mar 20, 2019

saturninoabril requested review from levb and removed request for saturninoabril March 20, 2019 14:58

levb previously requested changes Mar 20, 2019

View reviewed changes

components/view_image/view_image.jsx Outdated Show resolved Hide resolved

plugins/registry.js Outdated Show resolved Hide resolved

chetanyakan added 2 commits March 22, 2019 12:57

Code refactoring

cee15c1

Expost pdfjs to window to use in webpack externals in plugins

ea753fc

jasonblais requested review from levb, crspeller and jwilander and removed request for crspeller and levb March 22, 2019 13:00

jwilander reviewed Mar 22, 2019

View reviewed changes

chetanyakan added 2 commits March 22, 2019 23:34

- Pass postId to ViewImageModal and use getPost to fetch post object.

61575cb

- Update test cases

Merge branch 'master' of github.com:mattermost/mattermost-webapp into…

49d9685

… MI-373

hanzei requested review from jwilander and levb March 23, 2019 19:53

jasonblais removed the request for review from levb March 24, 2019 21:27

crspeller suggested changes Mar 25, 2019

View reviewed changes

jasonblais requested a review from crspeller March 25, 2019 13:29

jwilander approved these changes Mar 26, 2019

View reviewed changes

hmhealey added the CherryPick/Approved Meant for the quality or patch release tracked in the milestone label Mar 26, 2019

crspeller reviewed Mar 26, 2019

View reviewed changes

plugins/registry.js Show resolved Hide resolved

Update plugins/registry.js

53cfb18

Co-Authored-By: chetanyakan <[email protected]>

crspeller approved these changes Mar 27, 2019

View reviewed changes

crspeller merged commit 0b76c1e into mattermost:master Mar 27, 2019

chetanyakan deleted the MI-373 branch March 27, 2019 14:24

crspeller added CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone and removed 2: Dev Review Requires review by a core commiter CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Mar 27, 2019

amyblais added Changelog/Done Required changelog entry has been written Docs/Needed Requires documentation labels Mar 27, 2019

jasonblais added Docs/Done Required documentation has been written and removed Docs/Needed Requires documentation labels Apr 3, 2019

lindy65 added the Tests/Not Needed Does not require new release tests label Apr 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a plugin component to override file previews #2519

Create a plugin component to override file previews #2519

chetanyakan commented Mar 19, 2019

saturninoabril commented Mar 20, 2019 •

edited

Loading

hanzei commented Mar 20, 2019

levb left a comment

chetanyakan commented Mar 22, 2019

jasonblais commented Mar 22, 2019

jwilander Mar 22, 2019

chetanyakan Mar 22, 2019

crspeller Mar 25, 2019

jwilander Mar 26, 2019

jwilander Mar 22, 2019

chetanyakan Mar 22, 2019

crspeller Mar 25, 2019

crspeller Mar 25, 2019

chetanyakan Mar 25, 2019

crspeller Mar 25, 2019

chetanyakan Mar 26, 2019

crspeller Mar 26, 2019

amyblais commented Mar 26, 2019

crspeller commented Mar 26, 2019

chetanyakan commented Mar 27, 2019

		window.PropTypes = require('prop-types');
		window.PDFJS = require('pdfjs-dist');

Create a plugin component to override file previews #2519

Create a plugin component to override file previews #2519

Conversation

chetanyakan commented Mar 19, 2019

Summary

Ticket Link

Checklist

saturninoabril commented Mar 20, 2019 • edited Loading

hanzei commented Mar 20, 2019

levb left a comment

Choose a reason for hiding this comment

chetanyakan commented Mar 22, 2019

jasonblais commented Mar 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amyblais commented Mar 26, 2019

crspeller commented Mar 26, 2019

chetanyakan commented Mar 27, 2019

saturninoabril commented Mar 20, 2019 •

edited

Loading