Skip to content
This repository was archived by the owner on Aug 7, 2023. It is now read-only.

Conversation

@download13
Copy link
Contributor

Instead of running htmlhint as a subprocess, import and call it directly with the buffer contents. This enables lintOnFly.

Solves #111

subprocess.
lintOnFly can be enabled as a result.
No need for executable path config is there's no executable.
We are depending on an internal API. Incompatibilities could be introduced at any time.
lib/index.js Outdated
@@ -1,8 +1,13 @@
'use babel';

import * as fs from 'fs';
Copy link
Member

@Arcanemagus Arcanemagus May 17, 2016

Choose a reason for hiding this comment

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

If possible these modules should be lazily loaded, I've used lazy-req in the past, but if you have a better method go for it 😉.

Right now this will increase the package loading time as these will be required on Atom launch.

(This comment goes for all the imports.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed out all the imports for require calls in the places where they are needed. It looks a little odd, but it shaved about 20-30ms off the load time.

@download13
Copy link
Contributor Author

download13 commented May 18, 2016

Whoops. I responded to your comments but then they disappeared when I pushed the new commit.
Is that supposed to happen? Should I reply in this main thread thing?

@download13
Copy link
Contributor Author

Should I change anything else? Is there someone who needs to look this over before it can be merged?

@Arcanemagus
Copy link
Member

Comments on a specific line of code are collapsed when the underlying code in a PR is updated to make the comment no longer relevant to that line of code. It's a nice feature of GitHub so when fixes/changes are made to the code the comments that are (most likely) no longer relevant are hidden unless somebody explicitly looks for them.

lib/index.js Outdated
};

let executablePath = '';
export const config = {};
Copy link
Member

Choose a reason for hiding this comment

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

No need for this line.

@Arcanemagus
Copy link
Member

So the point I was trying to make with the lazy-req reference wasn't just to move the require calls around. The way it is now does move the load away from the package load time, but now the lints themselves will be longer, and we are relying on require to cache loads, which isn't always the case. With something like lazy-req the first use is just as painful as a regular require, but subsequent uses are guaranteed to be cached. The only issue I have with it is most things are turned into a function call which makes the code look a bit odd, but it works.

Since Atom is usually pretty good about caching require calls I'd be fine with merging this as is, after the above comment is addressed. Unless there is something else you want changed @johnwebbcole?

@johnwebbcole
Copy link
Contributor

I'm ok with it

@download13
Copy link
Contributor Author

When does require not cache loads? I thought it cached everything unless something explicitly deleted it's cache.

@download13
Copy link
Contributor Author

So it's ready to merge? Nothing else to do?

@Arcanemagus
Copy link
Member

Sorry, this must have gotten lost in my notifications 😞. Taking one more look over this and then merging.

@Arcanemagus
Copy link
Member

Published as v1.2.0, thanks again! 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants