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.

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

Update list of TypeScript/JavaScript interpreters #4470

Merged
merged 9 commits into from
May 10, 2019
Merged

Update list of TypeScript/JavaScript interpreters #4470

merged 9 commits into from
May 10, 2019

Conversation

Alhadis
Copy link
Collaborator

@Alhadis Alhadis commented Mar 22, 2019

@ahwayakchih's mention of Deno in #4469 reminded me that we haven't added it yet as a TypeScript/JavaScript interpreter, so here it is. Deno is a newer JavaScript runtime which has already gained traction for being a novel alternative to Node.js. It supports TypeScript out-of-the-box, making it a "true" dual JS/TS interpreter.

Template removed as it doesn't apply.

UPDATE: This PR's been updated to remove node from TypeScript's interpreters list as well, following conversation between @pchaigno and @grant. Merging this PR will now also fix #4469.

@pchaigno
Copy link
Contributor

@Alhadis Could you add a sample file with that interpreter? If we remove the node interpreter for TypeScript, this sample file will become the only test of the changes we made to allow several languages to have the same interpreter.

@Alhadis
Copy link
Collaborator Author

Alhadis commented Mar 22, 2019

@pchaigno Couldn't we just s/node/deno/ in the fixture's Hashbang? 😂

@pchaigno
Copy link
Contributor

Oh, right, it's a fixture one, so yes!

@Alhadis
Copy link
Collaborator Author

Alhadis commented Mar 22, 2019

I Deno if it was intentional or not, but the name "Deno" is an anagram of "Node". 😁

@ahwayakchih
Copy link

ahwayakchih commented Mar 22, 2019

I'm not sure if that fixture makes sense for Deno. It's true that Deno is a TypeScript interpreter, but it does not have the same "core library" API as node, as far as i know. So things like call to require(), or importing fs or path may not work in Deno at all.
Maybe it would be better to replace current TypeScript fixture with true TypeScript, generic code? Like one of the examples on TypeScript website? So it would include TypeScript specific syntax, instead of looking just like regular JavaScript file.
Or maybe, for example, use https://github.com/angular/angular/blob/master/packages/router/src/router.ts instead? It seems to contain type declarations, type casting, and some other stuff that's not valid in "pure" JavaScript, so maybe it could be more helpful for the classifier than the current fixture?

@Alhadis
Copy link
Collaborator Author

Alhadis commented Mar 22, 2019

This is a fixture file, it's only used for regression tests. The contents of test/fixtures aren't seen or used by the classifier, only the contents of samples are.

@ahwayakchih
Copy link

@Alhadis ah, my mistake, sorry and thanks for explanation :).

Copy link
Contributor

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

@Alhadis Could you also remove the node interpreter from TypeScript?

@Alhadis
Copy link
Collaborator Author

Alhadis commented Mar 27, 2019

@pchaigno Done. 👍

@Alhadis
Copy link
Collaborator Author

Alhadis commented Mar 28, 2019

@pchaigno Think now would be a good time to add other JS interpreters that we're missing?

JavaScript Engine Name(s) of interpreter program
Chakra chakra
SpiderMonkey js
Rhino rhino
V8 d8 (formerly v8-shell, v8, and then just d8)

Nothing "special" to any of these, they're just regular JS interpreters the same way Node.Js is.

@Alhadis
Copy link
Collaborator Author

Alhadis commented Apr 2, 2019

@pchaigno Heh, I just discovered ts-node. 😉 Definitely going on the list.

Pretty much damning proof that adding plain node as a TypeScript interpreter was a mistake.

@Alhadis Alhadis changed the title Add Deno to TypeScript/JavaScript interpreters Update list of TypeScript/JavaScript interpreters Apr 2, 2019
@@ -1,4 +1,4 @@
#!/usr/bin/env node
#!/usr/bin/env deno
Copy link

Choose a reason for hiding this comment

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

This file is not valid in Deno. (mostly due to imports)

  1. Deno is not Node so you can not import packages like 'fs'
  2. The file extension is important (./x.ts is valid but ./x is not)
  3. Deno does not use NPM, so import * as tsickle from 'tsickle'; is not right
    etc...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pchaigno Could we get away with using a "Hello, world" type script for the TypeScript/JavaScript hashbang fixtures? I mean, the content of the script is kind of irrelevant after all...

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, but if it's not too difficult to find a sample Deno file, it's even better. It'd help train the classifier to recognize the few differences of Deno.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ouch. Well, when I substitute the contents of test/fixtures/TypeScript/main for this TypeScript program, the tests fail, complaining that it identified as JavaScript. 😞

Do you think a heuristic is in order? Accurately differentiating TypeScript from JavaScript is impossible without a LOT of hairy regexes, but matching from "[^"]+\.ts" should work, since Node doesn't grok that extension.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've pushed my update. Have a look at the test feedback, I'm not really sure what's going on... 😢

Copy link
Member

Choose a reason for hiding this comment

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

I'm not really sure what's going on... 😢

You do 😉 ...

Accurately differentiating TypeScript from JavaScript is impossible without a LOT of hairy regexes

You've added deno to both JS and TS so things will fall through to heuristics and then the classifier and as there isn't a heuristic we're falling through to the classifier which due to the similarities of JS and TS has guessed this is JS not TS:

$ LINGUIST_DEBUG=1 bundle exec bin/github-linguist test/fixtures/TypeScript/main
main: 66 lines (55 sloc)
  type:      Text
  mime type: text/plain
JavaScript =  -1455.108 +  -4.009 =  -1459.117
TypeScript =  -1591.191 +  -5.727 =  -1596.918
  language:  JavaScript
  appears to be a vendored file
$

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hold up. I thought the point of these fixtures was to assert that the interpreter directives were recognisable?

Not gonna lie, the tests are completely and utterly opaque to me. 😢 Really not familiar with unit-testing in Ruby, but this is what I'm used to reading.

so things will fall through to heuristics and then the classifier and as there isn't a heuristic we're falling through to the classifier which due to the similarities of JS and TS has guessed this is JS not TS:

Alrighty, sooo... the only sane solution is to remove Deno as a JavaScript interpreter. Which is okay, really, because TypeScript interpreters can compile JS, just not the other way around. 😁

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's my fault for thinking too much about file extensions being tested before heuristics... of course, hashbangs are most relevant in files that don't have any extensions.

I'm an idiot, sorry. Deno's been pushed back into TypeScript-land. 👍 Hopefully the specs will pass now...

Alhadis added a commit to file-icons/atom that referenced this pull request Apr 24, 2019
@Alhadis
Copy link
Collaborator Author

Alhadis commented Apr 29, 2019

Note that this is now blocking another TypeScript-related PR. Any chance we could get this wrapped up?

/cc @lildude @pchaigno

@Alhadis Alhadis requested a review from lildude May 10, 2019 09:50
Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

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

LGTM. Not sure if @pchaigno has any final comments or thoughts though.

Copy link
Contributor

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

LGTM!

@Alhadis Alhadis merged commit cb9318e into master May 10, 2019
@Alhadis Alhadis deleted the deno branch May 10, 2019 10:12
@github-linguist github-linguist locked as resolved and limited conversation to collaborators Jun 17, 2024
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.

JavaScript (node.js) "executable" script gets misidentified as TypeScript
5 participants