Markdown preview

[TalAmuyal]

Based bryphe/proto/markdown-preview and merged origin/master onto it.
Next up: make something that works.

Should fix #49.

[bryphe]

Oops, I was using this as debugging to see what marked returns 😄

[bryphe]

A good next step would be to listen to the buffer update events here - we could do:

Oni.editors.activeEditor.onBufferChanged.subscribe((evt) => { console.log("Buffer Changed"; debugger; })

That way, if you have the debugger open, you can inspect evt and see the values for it. We can get the filename + filetype (language) and decide if we should render a preview for it.

If we should, we can get the content via the contentChanges array, and then render it out to our component using marked.

[TalAmuyal]

I'm done, but unfortunately the PR fails in CI on npm run dist:win (as well as other dists).
Any idea why or how it can be fixed?

[bryphe]

I'm done, but unfortunately the PR fails in CI on npm run dist:win (as well as other dists).
Any idea why or how it can be fixed?

Cool, it's the home stretch now! Can't wait to get this in!

I'm looking into the failure now.

[bryphe]

I just saw a couple issues that are unrelated to the failure, but they might cause tests to failure:

• It looks like there is a large h q file at the root, should be removed
• It looks like the submodules in vim/core and vim/default were removed. This will cause the typescript test to fail (because the filetype won't be picked up).
  

Those will need to be restored.

Still looking at the failure with npm run dist/npm run dist:win. I see other people have had similiar issues:
electron-userland/electron-builder#2236

But it looks like it might be an actual issue with a dependency. Trying to see what's happening.

[bryphe]

I tried setting the ELECTRON_BUILDER_ALLOW_UNRESOLVED_DEPENDENCIES and the build completed, and I was able to run the app.

We could try this on the CI machine:

    "postinstall": "npm run install:plugins && cross-env ELECTRON_BUILDER_ALLOW_UNRESOLVED_DEPENDENCIES=1 && electron-rebuild && opencollective postinstall"

However, once I got that, I tried running the app from the install package, and I see an issue:

Seems like the problem there is that react is a development dependency (which worked fine before, since the browser bundle was the only code that used it, and it bundled it with webpack). For this case, we should move it to a dependency as opposed to a devDependency at the root.

I'm going to try that out and see if there are any other blockers-

[bryphe]

Once I made that change, it worked in the dist build!

I pushed up a branch with the full set of fixes I used here: bryphe/922/fixes

[TalAmuyal]

Doesn't seem to solve it :(

[bryphe]

Ah looks like I messed up... I had set the variable in my shell so I was getting a false positive.

There were two problems with my original solution:

• The cross-env calls need to be prior to the build call in dist:win and pack, not in the postinstall step.
• The cross-env calls need to not have a && prior to the call, otherwise it's a no-op

Posted the fixes here in bryphe/922/move-cross-env-to-build-calls, and the relevant commit is this one:
d9ef232

Hopefully that's the last issue... fingers crossed...

[TalAmuyal]

Now there is a different error >< https://travis-ci.org/onivim/oni/jobs/317490353#L1019

[bryphe]

Ah well, at least the windows builds passed... making progress... 😄

[bryphe]

Taking a look at the OSX failure... that's strange. Seeing if I can repro it on my OSX machine.

[bryphe]

Strange, no issue on my machine. I couldn't find a lot of similiar issues, there were ones like this: electron-userland/electron-builder#1183

But not quite the exact same. Still looking.

[bryphe]

I'm wondering if it's having an issue with the nested dependencies in oni-markdown-preview. I'm going to test out promoting them to the root to see if that changes anything.

[bryphe]

I saw there was also an issue with the build caches - it seems that sometimes our node_modules / yarn caches get out of date 😦 I cleared them out, and I got a passing build with #1141 with the change to move the packages to the root: 7e64896

Unfortunately it seems that electron-builder doesn't do too well with the nested node_modules. It's not ideal, but we can put them at the root for now, and eventually move it out to its own npm module / repo.

[bryphe]

@TalAmuyal - let me know if you'd just prefer me to merge in #1141 . I wasn't sure if your name is associated with the squashed commit / if you get github activity in that case, so I was holding off. But I'm happy either way. Really excited to get this in 👍 !

[TalAmuyal]

Well, I tried a few more things, but no complete success.
It fails a bit further down the road (times-out on every ci tests * ci test test).
I don't think the issues originate from this PR.

If you don't mind, please do merge this PR and let's fix any ramifications independently.

[bryphe]

If you don't mind, please do merge this PR and let's fix any ramifications independently.

Sounds good - I'm in the midst of investigating some of the failures, so I'll take a look. I'll merge this in now - thanks for all your work on this, @TalAmuyal ! Congratulations on getting it, this is awesome 💯