Implement feature request #189 (Option to use global tslint instance) #196

smhxx · 2017-09-14T15:49:07Z

Howdy, all! 👋

I stumbled across #189 earlier today after wishing for the exact same feature... it's fairly simple and linter-eslint already has a similar option, so I decided it was worth wrangling their implementation into linter-tslint as well.

There were some minor linting issues with the current codebase due to recent versions of eslint (you can see what I'm talking about in the CI report for #195) so I took the initiative to resolve those as well. Two of them were super minor and well-founded, but the other one introduced by v4.6.0 was a bit silly (as others have mentioned,) so I went ahead and // eslint-disabled the relevant lines to avoid mucking the style up without a second opinion. Anyway, I hope this is helpful, and any feedback is of course welcome. 😃 👍

Arcanemagus

Can you also rebase this on master? I merged in #195 so the linting configuration is up to date now.

Arcanemagus · 2017-09-15T06:16:45Z

package.json

@@ -19,33 +19,53 @@
    "rulesDirectory": {
      "type": "string",
      "title": "Custom rules directory (absolute path)",
-      "default": ""
+      "default": "",
+      "order": 2


If you're going to add order to these, re-order them in package.json itself to match the order 😉.

Hah, I don't know why I didn't just do that to begin with. I guess I was a bit shy contributing to a new repo and trying to keep my "lines modified" count down. 😝 It's fixed!

Arcanemagus · 2017-09-15T06:17:54Z

package.json

+    },
+    "useGlobalTslint": {
+      "type": "boolean",
+      "title": "Use the global tslint install if there is no local version (or it is disabled)",


This should be short and sweet: Use the global TSLint install. Combine the rest into the description.

smhxx · 2017-09-15T22:19:12Z

Alright, I'll make those tweaks tonight, then rebase and update the PR. 👍

smhxx · 2017-09-16T01:17:23Z

Okay, changes made and squashed into the original commit! Should be in good shape now. 😄

Xapphire13

Looks good so far, I have just a few more changes for you though =]

Xapphire13 · 2017-09-17T16:10:38Z

lib/worker.js

@@ -93,6 +95,22 @@ async function getLocalLinter(basedir) {
  });
 }

+async function getNodePrefixPath() {


This function is marked async, but I don't see any async calls

Xapphire13 · 2017-09-17T16:12:07Z

lib/worker.js

+  const npmCommand = process.platform === 'win32' ? 'npm.cmd' : 'npm';
+  let nodePrefixPath;
+  try {
+    nodePrefixPath = ChildProcess.spawnSync(npmCommand, ['get', 'prefix'], {


We shouldn't use the *Sync versions of system commands, this takes processing time away from other atom packages due to us blocking on io

Whoops, my mistake! In my haste to port over linter-eslint's implementation, I forgot to adjust it for the fact that the rest of the code here is all nice and async'd. Didn't catch that they were using spawnSync(), although in retrospect it should have been obvious. xD I'll get right on that. 😅

Xapphire13 · 2017-09-17T16:14:04Z

package.json

+    "globalNodePath": {
+      "type": "string",
+      "title": "Global node installation path",
+      "description": "The location of your global npm install (`npm get prefix`). Optional.",


Maybe add (will default to `npm get prefix`)

smhxx · 2017-09-17T20:49:42Z

Alright, I went back and looked over worker.js and realized that I had actually made a couple mistakes there. I've gone ahead and made some corrections and squashed them into the original commit. The basics:

Got rid of the spawnSync() like you suggested, and replaced it with an exec(), so now the function that was mysteriously marked as async now actually is async. 😝
I realized that the way I was using getLocalLinter() was wrong and lazy, and as a result, the global linter wasn't being cached properly, potentially leading to npm get prefix being run every time the linter was requested. (Yikes.) I had to add a second parameter to that function to get it to work correctly for both the local & global linters, but hopefully the way I did that makes sense to you guys. I also renamed it to resolveAndCacheLinter() to give a clearer indication of the function's purpose (especially now that it's not just being used for finding the local package.)
Fixed a dumb typo. :x

I also went through with the Developer Tools up and ensured that it was using the correct linter, caching correctly, etc. and I'm now confident that it's working the way I intended. 👍

Xapphire13 · 2017-09-18T06:39:17Z

lib/worker.js

@@ -86,13 +89,30 @@ async function getLocalLinter(basedir) {
        } else {
          linter = tslintDef;
        }
-        tslintCache.set(basedir, linter);
+        tslintCache.set(fileDir, linter);


linterDir?

Xapphire13 · 2017-09-18T06:41:08Z

lib/worker.js

+      { env: Object.assign(Object.assign({}, process.env), { PATH: getPath() }) },
+      (err, stdout, stderr) => {
+        if (err || stderr) {
+          throw (err || new Error(stderr));


I haven't tried this before, does this bubble up correctly as if you used the reject of the promise?

Yeah, I juggled with doing it this way vs. using reject() and opted for this mainly because the try/catch in getLinter() looked cleaner to me than the alternative "two-callbacks" method. I suppose it's really personal preference and I'll defer to you guys if you have a preference one way or the other. It does work, though.

If an error is thrown in the executor function, the promise is rejected. The return value of the executor is ignored.

That's great! I totally agree it looks cleaner =]. I'll be adopting this style from now on I think.

Wait, just looked at it a little closer, the .exec() callback isn't guaranteed to be in the executor function though right? It may have a different call stack.

Y'know, you're right. It would normally work, but the fact that it's taking place in another process in this case means that needs to be a Promise.catch() rather than a regular try/catch block. I just tested it by changing npm to nom and with that change made, now it does catch the rejection properly. 👍 Will update in a second.

Xapphire13 · 2017-09-18T06:43:40Z

lib/worker.js

+    return resolveAndCacheLinter(basedir);
+  }
+
+  if (config.useGlobalTslint) {


Is basedir really used when using global tslint?

Currently, yes, although I suppose there may be a slightly more efficient way to do it? getLinter() is called with the path of the file being linted, and returns the instance of tslint that it's supposed to use for files in that directory, which depends on whether that directory has a local tslint package installed as a dependency. Since the local tslint (if there is one) still takes precedence over the global install, we cache the global tslint only for directories that we've confirmed don't have a local install. That way when directories we haven't checked yet show up, we still go through the check for the local install before going with the global or default one.

It would also be an option to separately cache the global linter after the first time we fetch it, to avoid having to resolve it again for each new directory, but ultimately it would need to be inserted into the tslintCache Map for each directory that we've confirmed doesn't have a local install, just like on line 109 where the same is done for the packaged "fallback" install.

smhxx · 2017-09-18T21:13:57Z

Heh, so this has turned into more changes and refactoring than I intended, but:

I've confirmed that npm get prefix now fails gracefully, simply outputting a message to console.warn() and then using the packaged version of tslint, with no hard errors thrown.
I went ahead and made that optimization I mentioned earlier, whereby the "fallback" linter is only resolved once, and then stored in a variable for use any time future directories don't have a local tslint install. If the "use global" flag is set, and it is able to find the global tslint install, it assigns that as the fallback; if the "use global" flag isn't set, or if it wasn't able to find the global install the first time, then it uses the packaged version as the fallback instead.

Xapphire13 · 2017-09-21T20:28:15Z

lib/worker.js

-}
-
-async function getLocalLinter(basedir) {
+async function resolveAndCacheLinter(fileDir, moduleDir) {


This doesn't need to be marked as async if no awaits are done within it

I think this is my last comment =]. @Arcanemagus, any objections after this?

Ah, true, since all it's doing is immediately returning a Promise, it doesn't really need to be marked as async. I'll go ahead and clean up those unnecessary async declarations on the functions that do that.

Just as a note, I personally leave the async on there just to make it obvious to myself that the function needs to be awaited on where it gets called, not a huge issue though as you are perfectly
right that it isn't needed 😉.

Arcanemagus · 2017-09-22T02:39:10Z

lib/worker.js

@@ -84,30 +80,78 @@ async function getLocalLinter(basedir) {
            linter = require('loophole').allowUnsafeNewFunction(() => require(linterPath).Linter);
          }
        } else {
-          linter = tslintDef;
+          return resolve();


🐛 You should just need to call resolve(), I'm not actually sure what return resolve() would do, I'm guessing that resolve() returns some value that makes that make sense as I've seen that elsewhere without issues, but the return isn't needed.

I noticed this too. There shouldn't be I'll effect, but for consistency I agree we should remove it

Good call. I was doing it (and I suspect others do it) in order to kick execution back up the stack immediately, hence avoiding the caching operation on line 85... but that doesn't really need to be outside of the if-block anyway, so moving that inside and then getting rid of the else seems like a cleaner option.

Arcanemagus · 2017-09-22T02:39:26Z

lib/worker.js

        }
-        tslintCache.set(basedir, linter);
+        tslintCache.set(fileDir, linter);


❓ Looks like @Xapphire13's earlier comment got marked resolved without being resolved:
Shouldn't this be baseDir?

Actually, that's correct as-is (at least, it's what I intended, "correct" is rather subjective 😛.) It specifically needs to be fileDir because the lookup on lines 110-113 searches the cache for the linter that was used the last time a source file in that directory was linted. If we use baseDir, which might actually be the user's npm directory, then it'll look up which linter to use every time and it will never actually get cached in a way that it can find later.

Ah ha, makes perfect sense. 👍

Arcanemagus · 2017-09-22T02:39:39Z

lib/worker.js

-
-async function getLocalLinter(basedir) {
+function resolveAndCacheLinter(fileDir, moduleDir) {
+  const basedir = moduleDir || fileDir;


🐛 baseDir please 😉

I wasn't sure how you wanted me to deal with that, since it previously was called basedir, apparently for the sake of brevity on line 71. I guess changing that line to { basedir: baseDir } wouldn't be the end of the world.

Arcanemagus · 2017-09-22T02:42:00Z

lib/worker.js

        return resolve(linter);
      },
    );
  });
 }

+function getNodePrefixPath() {
+  return new Promise((fulfill, reject) => {


🐎 This is extrememely unlikely to change during runtime, cache it after the first (successful) call and return that.

❓ Any particular reason you used fulfill instead of the standard resolve? I wouldn't block over it, but the non-standard nature of it requires a double take on the code.

The npm prefix doesn't actually have to be cached because this function is never called more than once. If it ever gets to the point that it needs to grab the prefix (because there was no local linter, and the user set the "use global tslint" option but didn't explicitly give their prefix in the settings) then it immediately uses the prefix to grab the global linter. If it's able to resolve it, then it caches that one as the "fallback linter," and if it's not able to resolve it, it caches linter-tslint's version as the "fallback linter." Either way, this function should never be called again, because getLinter() will never get past line 124 once fallbackLinter is set. 👍

As for the fulfill/resolve thing, I'm not actually sure... I thought maybe I had seen it in linter-eslint or something and copied the style, but now I can't find it anywhere. Dunno where I got it from, to be honest. xD I'll just change it to resolve, I don't really care either way and resolve does seem more "standard," as you said.

Makes sense about the calling of getNodePrefixPath, thanks for clearing that up!

resolve is actually part of the standard documentation (Promise), but you can use whatever name you want... almost everyone just uses the standard names though lol.

Arcanemagus · 2017-09-22T02:47:12Z

lib/worker.js

+    }
+
+    const globalLint = await getNodePrefixPath()
+      .then(prefix => resolveAndCacheLinter(basedir, prefix))


🐛 Why is this using Promise syntax? Nothing in here is a callback function that requires a Promise wrapper to work...

I guess the .then() isn't 100% necessary, I just figured it was easier to chain the two promises and await them together, but since resolveAndCacheLinter() never rejects, I guess it doesn't really have to be before the catch(). It could also be done like:

const prefix = await getNodePrefixPath().catch((err) => { ... }); if (prefix) { const globalLint = await resolveAndCacheLinter(basedir, prefix); fallbackLinter = globalLint; return globalLint; }

I guess it's just preference as to which makes the flow more obvious? I'd be okay with either.

Arcanemagus · 2017-09-22T02:48:33Z

lib/worker.js

+The error message encountered was:\n\n${err.message}`);
+      });
+    if (globalLint) {
+      fallbackLinter = globalLint;


❓ Should this be cached in tslintCache?

I would probably leave fallbackLinter in its own variable, personally? I mean, you could put it into the map with everything else, but since it could technically be the global linter or the linter-tslint packaged version, that sorta makes finding it again a little complicated. This way, it's just "No local linter? Alright, use the other one." and you're done.

Makes sense. 👍

Arcanemagus · 2017-09-22T02:50:31Z

lib/worker.js

-}
-
-async function getLocalLinter(basedir) {
+async function resolveAndCacheLinter(fileDir, moduleDir) {


Just as a note, I personally leave the async on there just to make it obvious to myself that the function needs to be awaited on where it gets called, not a huge issue though as you are perfectly
right that it isn't needed 😉.

Arcanemagus

Here's what I see, key for the comments:
🐛 Definitely looks like something is wrong
🐎 Nothing wrong, but it should be changed for performance
❓ Maybe something wrong, or just a question/note.

Looks like this is getting close though!

Arcanemagus · 2017-09-22T02:54:29Z

Well that's annoying, it posted all my pending review things when I made the single comment here 😞.

Xapphire13 · 2017-09-22T03:48:07Z

@Arcanemagus, GitHub won't let me response to your comment about async. Isn't the presence of async without an await illegal in TypeScript? I know it is in C#. I felt like since this lint TypeScript, we should align with its rules =]. Though I'm also not passionate about it haha.

This begs the question, should this project be rewritten in TypeScript? =]

Arcanemagus · 2017-09-22T05:01:35Z

Isn't the presence of async without an await illegal in TypeScript?

Fascinating, I wonder if that's an artifact of TypeScript's method of transpilation to ES5? It's not necessarily a bug in JavaScript, it's technically a bit of overhead though as initializing an async function is more expensive as a regular function.

In any case, like I said it's just a handy notation, it's not important so leaving it out is just fine.

This begs the question, should this project be rewritten in TypeScript?

I'm not against it, I have no experience with it though 😛.

Arcanemagus · 2017-09-22T05:07:55Z

lib/worker.js

  return new Promise((resolve) => {
    if (!requireResolve) {
      requireResolve = require('resolve');
    }
    requireResolve(
      tslintModuleName,
-      { basedir },
+      { basedir: baseDir },


Oh, didn't see that it was being sent as a parameter here. I guess we should probably keep this as basedir then. Sorry!

@smhxx Oh, this was supposed to be in the requested changes below.

Arcanemagus · 2017-09-22T05:10:33Z

lib/worker.js

        }
-        tslintCache.set(basedir, linter);
+        tslintCache.set(fileDir, linter);


Ah ha, makes perfect sense. 👍

Arcanemagus

🤦‍♂️

Arcanemagus

One last little thing!

Arcanemagus · 2017-09-22T05:18:24Z

lib/worker.js

+      }
+    }
+
+    const prefix = await getNodePrefixPath().catch((err) => {


This should be in a try/catch block:

let prefix try { prefix = await getNodePrefixPath(); } catch (err) { // eslint-disable-next-line no-console console.warn(`Attempted to load global tslint, but \`npm get prefix\` failed. Falling back to the packaged version of tslint. You can specify your prefix manually in the settings or linter-tslint config file.\n The error message encountered was:\n\n${err.message}`); }

That's what I meant as part of the "Promise syntax", sorry for not making that clear 😉.

Ah! The .catch(). 😛 Now I understand. Should be easy since that the two promises are separated already.

Arcanemagus · 2017-09-22T05:19:28Z

Sorry about being all over the place with the reviews, apparently I can't remember comments post pending review comments tonight.

smhxx · 2017-09-22T06:11:12Z

As for TypeScript, the compiler will complain if you have an async function with a return type that doesn't satisfy Promise<T>, but afaik it won't error if the callback of that Promise doesn't actually do any async. I guess they trust you enough that if your function is explicitly returning a Promise, you're probably smart enough to know whether you need it or not. 😛

Going from JavaScript -> TypeScript is pretty easy if you aren't doing any wonky wtf-level polymorphism that would make Douglas Crockford cry. Making the jump probably wouldn't be as bad as it sounds, although I suppose the question is "why," if it's just gonna have to be compiled back into JS for apm anyway. xD

Arcanemagus · 2017-09-22T06:14:28Z

I suppose the question is "why," if it's just gonna have to be compiled back into JS for apm anyway. xD

Type checking is definitely a good thing, with flow at least we can use babel which means the possibility of having Atom do the transpilation on install (configuration). I'm guessing that somebody has probably written a TypeScript transpiler plugin.

Arcanemagus

Last bit that I see

Arcanemagus · 2017-09-22T06:16:32Z

lib/worker.js

  return new Promise((resolve) => {
    if (!requireResolve) {
      requireResolve = require('resolve');
    }
    requireResolve(
      tslintModuleName,
-      { basedir },
+      { basedir: baseDir },


Looks like this got lost in the mess of comments, since this name needs to be sent as an object into requireResolve, let's just keep it as basedir in the entire function. Sorry for going back and forth on this!

woops! It's okay, I'll change it back. haha

Added new option to config & settings to use the global installation of tslint as a fallback, rather than the version included with linter-tslint.

Xapphire13 · 2017-09-22T07:06:43Z

Isn't the presence of async without an await illegal in TypeScript?

I was wrong, I think this may just be a C#-ism (which returns warning CS1998: Async block lacks `await' operator and will run synchronously)

I suppose the question is "why," if it's just gonna have to be compiled back into JS for apm anyway. xD

I would say for type checking and features that JS (and ES6/7) doesn't have yet. It may also yield a slight perf. increase since it would no longer be transpiled ES6+ running through babel at runtime. @Arcanemagus, we could get the TypeScript to compile upon install using scripts: {postinstall: "tsc"} or similar in the package.json file right?

smhxx · 2017-09-22T07:14:49Z

I bet you could even do it as a prepublishOnly script if you wanted to, and have users just download the compiled JS straight from ~~npm~~ apm. Not sure what the trade-off is between that and doing it at install, but I guess it's an option...

Xapphire13 · 2017-09-22T07:23:03Z

Looks like apm doesn't support prepublish =[ atom/apm#372

Arcanemagus · 2017-09-22T07:39:04Z

transpiled ES6+ running through babel at runtime.

Atom has a compileCache, when a file is first loaded it checks if it needs to be transpiled. If it does it does the work then and then stores the transpiled version (with source map) in the compileCache. Whenever the file is loaded after that it loads it instead from the compileCache directly.

tl;dr it only transpiles files once 😉.

@Arcanemagus, we could get the TypeScript to compile upon install using scripts: {postinstall: "tsc"} or similar in the package.json file right?

Not quite (for the reason @smhxx found). If you want to use transpiled code you either have to store both the source and transpiled version in the repository... or use the atomTranspilers key with a compatible package to control how Atom builds files for the compileCache. atom-babel6-transpiler is one written for babel. I don't know if anybody has bothered to create one for TypeScript yet though...

Arcanemagus

LGTM, thanks for sticking with this! ❤️

smhxx · 2017-09-24T00:55:10Z

@Arcanemagus Surprisingly, I looked and wasn't able to find one anywhere. Really surprises me since I'd imagine plenty of people are probably interested in being able to use TS to write Atom packages.

~70 LOC later, the problem has been resolved. 😉

EDIT: Of course, now the problem is that @types/atom is terribly out-of-date. Nothing's ever easy, is it? lol

Xapphire13 · 2017-09-24T07:36:07Z

@smhxx, awesome job! I was thinking about making something similar myself =].

Arcanemagus · 2017-09-25T16:42:22Z

Btw, Atom actually has native support for .ts files.... but uses some positively ancient version of typescript for it so it's near useless. Nice job on creating atom-ts-transpiler!

Arcanemagus suggested changes Sep 15, 2017

View reviewed changes

Arcanemagus added the enhancement label Sep 15, 2017

Xapphire13 suggested changes Sep 17, 2017

View reviewed changes

Xapphire13 reviewed Sep 18, 2017

View reviewed changes

Xapphire13 reviewed Sep 21, 2017

View reviewed changes

Xapphire13 approved these changes Sep 21, 2017

View reviewed changes

Arcanemagus reviewed Sep 22, 2017

View reviewed changes

Arcanemagus suggested changes Sep 22, 2017

View reviewed changes

Arcanemagus reviewed Sep 22, 2017

View reviewed changes

Arcanemagus suggested changes Sep 22, 2017

View reviewed changes

Implementing feature request #189 (global tslint)

9d910ab

Added new option to config & settings to use the global installation of tslint as a fallback, rather than the version included with linter-tslint.

Arcanemagus approved these changes Sep 22, 2017

View reviewed changes

Arcanemagus merged commit ee9af70 into AtomLinter:master Sep 22, 2017

Implement feature request #189 (Option to use global tslint instance) #196

Implement feature request #189 (Option to use global tslint instance) #196

Conversation

smhxx commented Sep 14, 2017

Arcanemagus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smhxx commented Sep 15, 2017

smhxx commented Sep 16, 2017

Xapphire13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smhxx commented Sep 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smhxx commented Sep 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arcanemagus left a comment

Choose a reason for hiding this comment

Arcanemagus commented Sep 22, 2017

Xapphire13 commented Sep 22, 2017

Arcanemagus commented Sep 22, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arcanemagus left a comment • edited

Choose a reason for hiding this comment

Arcanemagus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arcanemagus commented Sep 22, 2017

smhxx commented Sep 22, 2017

Arcanemagus commented Sep 22, 2017

Arcanemagus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xapphire13 commented Sep 22, 2017

smhxx commented Sep 22, 2017 • edited

Xapphire13 commented Sep 22, 2017 • edited

Arcanemagus commented Sep 22, 2017

Arcanemagus left a comment

Choose a reason for hiding this comment

smhxx commented Sep 24, 2017 • edited

Xapphire13 commented Sep 24, 2017

Arcanemagus commented Sep 25, 2017 • edited

Arcanemagus commented Sep 22, 2017 •

edited

Arcanemagus left a comment •

edited

smhxx commented Sep 22, 2017 •

edited

Xapphire13 commented Sep 22, 2017 •

edited

smhxx commented Sep 24, 2017 •

edited

Arcanemagus commented Sep 25, 2017 •

edited