added optional window to avoid requiring 'jsdom' everytime

[akhoury]

I added an optional options.window so I can now require('jsdom') and createWindow() before hand

kinda like that.

var posts = [ .... ]; // a large array of html content posts
var md = require('html-md');
var doc = require('jsdom').jsdom(null, null, {
          features: {
            FetchExternalResources: false
          },
          url: window ? window.document.baseURI : "file://" + (process.cwd()) 
        });
var win = doc.createWindow();

for (var i = 0; i < posts.length; i++) {
   posts[i].content = md(posts[i].content), {window: win});
}

I built and successfully tested locally, but didn't commit build files per your contribution guide
Thanks, great module btw.

closes issue#42

[neocotic]

Could you please rephrase this to not include the issue reference? For example:

# Use the `window` option when specified or create a DOM if `window` doesn't exist (i.e. when running in node).

[neocotic]

Although this does seem a lot like supporting a workaround instead of implementing a fix, I'm not sure how much this is a problem. I doubt the majority of use-cases will encounter the memory leak mentioned in #42 but I'm happy to accommodate you advanced users 😉

I would have preferred to only call jsdom(markup, level, options) once during the first call but, unfortunately, I see no way of changing the url of an existing instance, so a new one is created each time. Setting this URL is required in order to support resolving relative URL's in the markup.

If you could make the changes I've suggested, I'll merge it in and update the README and maybe get a minor release out.

[akhoury]

you were right, the bottleneck still in jsdom(markup, level, options) so I couldn't do more than 'requiring' jsdom once. Also updated the comment and another statement per your notes.

Thanks

[akhoury]

@neocotic any plans to merge that in?
just a friendly reminder, thank you - let me know if you need more changes.

[neocotic]

Yes, sorry. Thanks for the reminder 😄

[akhoury]

don't mean to nag, ✌️ just another friendly reminder.

[neocotic]

@akhoury Don't worry. I've not forgotten. Honestly, I'm currently looking at alternatives to jsdom since it's possibly too much overhead (especially for Windows users). Please stay with me and, if the investigation takes too long, I'll simply merge this in for the time being, at least.

Thanks for your patience.

[neocotic]

I haven't forgotten 😄 Will probably merge this in this weekend (have set a reminder).

[toddself]

@neocotic getting this into html-md would be awesome -- I'm trying to use html-md in a loop during a data migration here at work, and am running into this leak.

[akhoury]

@toddself - I have full confidence in @neocotic to address this corner-case issue one way or another, but till then, I was able to get away using this temporary close-enough-fork: (see why it's not a regular fork)

package.json:

"dependencies": {
        "html-md-optional_window": "git://github.com/akhoury/html-md-optional_window.git#master"
    }

then follow the example in the initial post of this thread for an example on how to use it.

IMPORTANT:

as I state in the readme, I WILL delete the repo once this issue is resolved.

[toddself]

@akhoury I saw that :) and resolved with an npm i akhoury/html.md#ff7bee1d11291c17b698b53f8b922d8108e45cf7

However, still getting a Heap::ReserveSpace Allocation failure. :(. I'm going to profile this and see if I can't find some other heap issues.

[akhoury]

can you provide an example for me to replicate the heap issue? I'd love to take a look.

[toddself]

Working on reducing down my example to something manageable :)

[neocotic]

I was looking at doing this this week (honestly!)

@akhoury, can you please coordinate with @toddself to see if there's any other changes you need to make your PR "fully featured". I'm hesitant to merge this in if it doesn't fix the core problem with this edge case.

[akhoury]

^ fair enough - still waiting on @toddself's reduced example to reproduce the heap issue.

[neocotic]

@toddself, any update on the problems you were experiencing?

[neocotic]

@akhoury, @toddself, I was wondering if there is any update on this as I'm keen to get this edge case fixed and behind us?

[akhoury]

@neocotic I was not able to reproduce the heap issue that @toddself mentioned.
It's your call.

[neocotic]

Sorry for taking so so very long to address this. I finally got around to doing the massive rewrite of this library that I wanted to. It's been renamed Europa and it's totally changed.

One key change that fixes this issue is that I've split up the project into several modules. europa is now only for browsers (and browser bundlers - e.g. browserify, webpack) and node-europa is now available for Node.js. This allows all Node.js-specific dependencies to be completely separated from that for the browser version. node-europa also contains the node-powered CLI.

I also bumped the dependency for jsdom to v11. While this means that node-europa requires Node.js version 6 or newer to be compatible with the same requirement on jsdom, it means no more contextify (yay! 🎆) This should be better for Windows users as well, who often had issues during the installation due to the somewhat complex external dependencies for contextify.

Most of all, if you want to install for browser or Node.js, now all you need is npm:

# For browser:
$ npm install --save europa
# For Node.js:
$ npm install --save node-europa

Both install quickly and have no confusing dependencies or external dependencies.

I'm closing this PR now as it's no longer valid. If you still use html-md, please give node-europa a shot and let me know how you get on.

Sorry again for the massive delay.