Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Refactoring Markdown lifecycle methods to React 16.3 #19436

Merged
merged 6 commits into from Jun 13, 2018
Merged

Refactoring Markdown lifecycle methods to React 16.3 #19436

merged 6 commits into from Jun 13, 2018

Conversation

markov00
Copy link
Member

@markov00 markov00 commented May 25, 2018 •
edited

This PR updates the Markdown component to new React 16.3 lifecycle methods as requested in #17432.

It removes completely the componentWillReceiveProps method in favour of a memoized markdownFactory method. The markdownFactory now use whiteListedRules and openLinksInNewTab as the cache key and return a callable function for rendering a markdown string (avoiding a possible state change of the markdown-it instance).
The memoized function is used by the markdown vis and the tutorial components, so a maximum of 3 instances will coexist on Kibana.

Markdown is now a PureComponent.

@markov00 markov00 added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) Feature:Markdown Markdown visualization feature v7.0.0 refactoring WIP Work in progress labels May 25, 2018
@markov00 markov00 requested review from timroes and ppisljar May 25, 2018 09:37
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

ppisljar
ppisljar reviewed May 25, 2018
View reviewed changes
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only comment i have is that the new code is harder to read than the old one, looking at the issues you mentioned it seems others are coming to the same conclusion ....

@markov00
Copy link
Member Author

@ppisljar Yes, it's a bit harder to read, and we are duplicating props into state. I can get rid of the initialization of markdown in the constructor since getDerivedStateFromProps is called also before the first rendering, but I've to fix all the markdown_vis tests because of the shallow vs mount issue.

Other possible solutions are:

  • use a different markdown library (some of them exists in as pure react implementation)
  • memoize the markdownFactory (it doesn't cost too much since we only have openLinksInNewTab that is a boolean and whiteListedRules that is never updated from the editor) create its instance and call the transform method on the render method without passing through setting the state

@markov00 markov00 force-pushed the refactoring-newlifecycler163 branch 3 times, most recently from 4e14ea8 to 54dfe78 Compare May 28, 2018 16:16
@markov00 markov00 force-pushed the refactoring-newlifecycler163 branch from 54dfe78 to d55dc42 Compare May 29, 2018 13:03
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@markov00
Copy link
Member Author

@timroes @ppisljar I've updated the PR and the description, can you take a look at the new Markdown refactoring? Seems a bit more readable than the version with the getDerivedStateFromProps.

timroes
timroes reviewed Jun 6, 2018
View reviewed changes
src/ui/public/markdown/markdown.js Outdated

export class Markdown extends PureComponent {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest, we just use a pure function component instead inheriting PureComponent here. Functional of course no difference, but I think it reads a bit cleaner and aligns better with what we usually do in components.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using a PureComponent to take advantage of its shallow compare of props. This avoids calling the render method if the parent rerenders and the props aren't changed as opposed to what happens when using a pure function that renders every time its parent renders independently from the props and state.
We have the markdownit render method in the render method of the pure component so it's better to don't rerender everything every time.

src/ui/public/markdown/markdown.js Outdated
});
}
}
return { __html: markdownIt.render(markdown) };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should rather return only the rendered markdown from this function, and wrap it in { __html } in the component below. That way the vanilla JS function stays less related to React.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok will do ;)

ppisljar
ppisljar approved these changes Jun 7, 2018
View reviewed changes
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, tested on chrome-linux

@markov00 markov00 force-pushed the refactoring-newlifecycler163 branch from d55dc42 to 96537a7 Compare June 12, 2018 10:24
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@timroes timroes requested a review from nreese June 12, 2018 12:04
nreese
nreese approved these changes Jun 12, 2018
View reviewed changes
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

code review
tested markdown rendering on home page "add data"

src/ui/public/markdown/markdown.js Outdated
* into HTML. It will just return an empty string when the markdown is empty.
* Since we want to use this with dangerouslySetInnerHTML, this method returns
* the required object format with an __html key in it.
* @param {String} markdown - Rhe markdown String
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo "Rhe" should be "The"

@markov00 markov00 force-pushed the refactoring-newlifecycler163 branch from 96537a7 to de3c3f8 Compare June 12, 2018 16:04
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@markov00 markov00 force-pushed the refactoring-newlifecycler163 branch from de3c3f8 to 8624cc3 Compare June 13, 2018 08:56
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@markov00 markov00 force-pushed the refactoring-newlifecycler163 branch from 8624cc3 to eb4a633 Compare June 13, 2018 10:17
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@markov00 markov00 force-pushed the refactoring-newlifecycler163 branch from eb4a633 to 1e3a0ea Compare June 13, 2018 11:47
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@markov00 markov00 merged commit 49724b2 into elastic:master Jun 13, 2018
@markov00 markov00 mentioned this pull request Jun 13, 2018
Merged
markov00 added a commit to markov00/kibana that referenced this pull request Jun 13, 2018
@markov00
Refactoring Markdown lifecycle methods to React 16.3 (elastic#19436)
f744f8b 
* Removed componentWillReceiveProps

* Memoized markdown factory

* Refactored some tests
markov00 added a commit that referenced this pull request Jun 13, 2018
@markov00
Refactoring Markdown lifecycle methods to React 16.3 (#19436) (#19855)
8a5ff4f 
* Removed componentWillReceiveProps

* Memoized markdown factory

* Refactored some tests
@markov00 markov00 deleted the refactoring-newlifecycler163 branch June 13, 2018 15:09
maryia-lapata pushed a commit to maryia-lapata/kibana that referenced this pull request Jun 25, 2018
@markov00 @maryia-lapata
Refactoring Markdown lifecycle methods to React 16.3 (elastic#19436)
0b48d1a 
* Removed componentWillReceiveProps

* Memoized markdown factory

* Refactored some tests
@elastic elastic deleted a comment from elasticmachine Aug 1, 2018
@elastic elastic deleted a comment from elasticmachine Aug 1, 2018
@elastic elastic deleted a comment from elasticmachine Aug 1, 2018
@elastic elastic deleted a comment from elasticmachine Aug 1, 2018
@markov00 markov00 mentioned this pull request Aug 1, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timroes timroes timroes left review comments

@nreese nreese nreese approved these changes

@ppisljar ppisljar ppisljar approved these changes

Assignees
No one assigned
Labels
Feature:Markdown Markdown visualization feature Feature:Visualizations Generic visualization features (in case no more specific feature label is available) refactoring v6.4.0 v7.0.0 WIP Work in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@markov00 @elasticmachine @nreese @timroes @ppisljar