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

[5.6] fork marked and remove npm marked dependency #13444

Merged
merged 2 commits into from
Aug 11, 2017
Merged

[5.6] fork marked and remove npm marked dependency #13444

merged 2 commits into from
Aug 11, 2017

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Aug 10, 2017 •
edited
Loading

fixes #13426

This PR forks the marked dependency and moves the forked version under src/forked/marked.

@nreese nreese added the v5.6.0 label Aug 10, 2017
kobelb
kobelb requested changes Aug 10, 2017
View reviewed changes
Copy link
Contributor

@kobelb kobelb left a comment

Choose a reason for hiding this comment

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

There are quite a few files that are candidates to being deleted, as they're something that we won't be using anymore with the way that marked is being imported/built.

Also, one of the tests for marked is failing right now, and I'd be interested to see if it could be integrated into our current test run as we're owning it now.

src/forked/marked/Gulpfile.js Outdated
@@ -0,0 +1,22 @@
var gulp = require('gulp');
Copy link
Contributor

Choose a reason for hiding this comment

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

This Gulpfile will never be ran, as we're referencing the lib/marked from everything else, and using our own build process.

src/forked/marked/.travis.yml Outdated
@@ -0,0 +1,5 @@
language: node_js
Copy link
Contributor

Choose a reason for hiding this comment

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

This file should probably be removed, we won't be using it.

src/forked/marked/.gitignore Outdated
@@ -0,0 +1 @@
node_modules/
Copy link
Contributor

Choose a reason for hiding this comment

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

This is redundant, as the .gitignore in the project root already has this.

src/forked/marked/.npmignore Outdated
@@ -0,0 +1,2 @@
.git*
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unused as we won't be publishing a npm module for this.

src/forked/marked/Makefile Outdated
@@ -0,0 +1,12 @@
all:
Copy link
Contributor

@kobelb kobelb Aug 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This should probably be deleted as well as we won't be building using make but our own build process.

src/forked/marked/bin/marked Outdated
@@ -0,0 +1,187 @@
#!/usr/bin/env node
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the benefit to leaving this here either.

src/forked/marked/bower.json Outdated
@@ -0,0 +1,24 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

The bower.json/component.json and package.json should probably be deleted as well, as this won't be used as a separate module. The devDependencies appear to be only used by the Gulpfile.js which I think should be deleted as well.

src/core_plugins/markdown_vis/public/markdown_vis_controller.js Outdated
@@ -1,4 +1,4 @@
import marked from 'marked';
import marked from '../../../forked/marked/lib/marked';
Copy link
Contributor

@kobelb kobelb Aug 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

We should be able to shorten this, and the other imports, to ../../../forked/marked

kobelb
kobelb approved these changes Aug 10, 2017
View reviewed changes
Copy link
Contributor

@kobelb kobelb left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM

As discussed "offline", leaving the tests separate seems fine since this will only be on a maintenance branch, and the integration would take too much effort.

epixa
epixa approved these changes Aug 11, 2017
View reviewed changes
Copy link
Contributor

@epixa epixa left a comment

Choose a reason for hiding this comment

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

LGTM

@uboness
Copy link

uboness commented Aug 11, 2017

nice!

@nreese nreese merged commit d764ab1 into elastic:5.6 Aug 11, 2017
nreese added a commit to nreese/kibana that referenced this pull request Aug 11, 2017
@nreese
[5.6] fork marked and remove npm marked dependency (elastic#13444)
9459a74 
* fork marked and swap for npm marked dependency

* remove unneeded files in marked fork, fix broken test in marked fork, shorten marked import path
This was referenced Aug 11, 2017
Merged
Merged
nreese added a commit that referenced this pull request Aug 11, 2017
@nreese
[5.6] fork marked and remove npm marked dependency (#13444) (#13468)
d1d762c 
* fork marked and swap for npm marked dependency

* remove unneeded files in marked fork, fix broken test in marked fork, shorten marked import path
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kobelb kobelb kobelb approved these changes

@epixa epixa epixa approved these changes

Assignees
No one assigned
Labels
v5.6.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nreese @uboness @epixa @kobelb