Add typescript-eslint-parser #242

josephfrazier · 2017-06-18T17:04:22Z

I adapted this from the babel-eslint parser, as described in
https://github.com/fkling/astexplorer/tree/b7ae2f06322c080313fdbea13501a19750d741ca#how-to-add-a-new-parser

This fixes #218.

I adapted this from the `babel-eslint` parser, as described in https://github.com/fkling/astexplorer/tree/b7ae2f06322c080313fdbea13501a19750d741ca#how-to-add-a-new-parser This fixes fkling#218.

RReverser · 2017-06-19T12:38:57Z

website/src/parsers/js/typescript-eslint-parser.js

+  locationProps: new Set(['loc', 'start', 'end', 'range']),
+
+  loadParser(callback) {
+    require(['typescript-eslint-parser'], typescriptEslint => callback(typescriptEslint));


Nit: no need to create extra arrow function here, you can use just require(['typescript-eslint-parser'], callback) as we do in many other places.

RReverser · 2017-06-19T12:39:20Z

website/src/parsers/js/typescript-eslint-parser.js

+import pkg from 'typescript-eslint-parser/package.json';
+
+const ID = 'typescript-eslint-parser';
+const name = 'typescript-eslint-parser';


You can just use displayName: ID, it's fine when they match (we do this in other parsers too).

RReverser · 2017-06-19T12:39:56Z

website/src/parsers/js/typescript-eslint-parser.js

+  },
+
+  nodeToRange(node) {
+    if (typeof node.range !== 'undefined') {


Perhaps just if (node.range) would be better since it theoretically can be(come) null.

See fkling#242 (comment)

josephfrazier · 2017-06-19T14:27:31Z

Thanks for the feedback, @RReverser! I just pushed up some fixes.

RReverser · 2017-06-19T17:55:47Z

website/src/parsers/js/typescript-eslint-parser.js

+    };
+
+    const ast = parser.parse(code, opts);
+    delete ast.tokens;


Does typescript-eslint-parser return .tokens? As far as I see, it does only when you pass explicit option to do so, so there's no point in deleting this property, you can just return result as-is.

RReverser · 2017-06-19T17:57:14Z

Thanks! Added one more comment and - can you please add settings for options that make sense in typescript-eslint-parser such as tolerant and ecmaFeatures.jsx (https://github.com/eslint/typescript-eslint-parser/blob/master/parser.js#L76-L83) and maybe some others?

See fkling#242 (comment)

josephfrazier · 2017-06-19T23:25:38Z

Done, let me know what you think.

RReverser · 2017-06-20T10:27:31Z

website/src/parsers/js/typescript-eslint-parser.js

+    {
+      key: 'ecmaFeatures',
+      title: 'ecmaFeatures',
+      fields: Object.keys(defaultOptions.ecmaFeatures),


Can you show how this looks in UI please? I can't remember, just know that in acorn settings I had to flatten properties myself.

Sure, it looks like this:

Nice, I should use it in the other place as well then :) Didn't know nested options work (or maybe they didn't when I added it).

One caveat I noticed is that if you have a non-nested option after the nested one(s), it will also appear to be nested. Probably just a simple CSS fix needed, though.

@josephfrazier I guess not as much CSS, as reordering of properties (render non-nested always first). Although I don't think it's a big deal, people can order properties theirselves when adding parsers, so should be fine.

RReverser · 2017-06-20T14:42:24Z

website/src/parsers/js/typescript-eslint-parser.js

+        settings => settings.ecmaFeatures || {...defaultOptions.ecmaFeatures},
+    },
+  ],
+};


I think you need to pass required= with range option (see

astexplorer/website/src/parsers/lua/luaparse.js

Line 24 in a4edbaa

required: new Set(['ranges']),

), otherwise it will be possible to disable range option which will render highlighter useless (nodeToRange will not work at all).

Alternatively, check if typescript-eslint-parser returns start & end properties on the nodes (like Acorn-derived parsers do), in which case use them for nodeToRange instead of .range itself.

Ah, good point. Fixed in 4912cfb

See fkling#242 (comment)

RReverser · 2017-06-20T15:32:32Z

website/src/parsers/js/typescript-eslint-parser.js

+
+  nodeToRange(node) {
+    if (node.range) {
+      return [node.range[0], node.range[1]];


Btw, can't just do return node.range here instead of entire condition?

Heh, that's what I get for doing super-focused edits :P Fixed in 49f9ff9

See fkling#242 (comment)

fkling · 2017-06-21T02:17:42Z

@RReverser: please merge whenever you think its ready (and add a the label "needs site push" so that I know that I have to push a new version 😉 )

RReverser · 2017-06-21T14:38:54Z

@fkling I want to hold onto this just a little bit, give me time for final reviews :)

RReverser · 2017-06-21T16:47:41Z

website/src/parsers/js/typescript-eslint-parser.js

+  loc: false,
+  tokens: false,
+  comment: false,
+  tolerant: true,


Perhaps should be false by default? (so that user would see proper syntax errors)

Oh, I had copied that from espree. Should we change it there too? It looks like esprima and recast already default to false.

Hmm, yeah, I would say it's a good idea to have visible errors by default.

See fkling#242 (comment)

RReverser · 2017-06-23T12:36:22Z

@josephfrazier Travis production build failed - can you please fix it (by making sure dependency is transpiled, see webpack.config.js)?

azz · 2017-06-25T06:17:57Z

website/src/parsers/js/typescript-eslint-parser.js

+    return parser.parse(code, {...defaultOptions, ...options} );
+  },
+
+  nodeToRange(node) {


The default implementation is the same (you can delete this)

See fkling#242 (comment)

josephfrazier · 2017-06-27T21:03:51Z

@RReverser, this should be good to go now. Thanks!

RReverser · 2017-06-27T21:21:07Z

@josephfrazier Yup, CI looks green. Thanks for your patience!

josephfrazier · 2017-06-27T21:22:00Z

Thanks for the merge!

fkling · 2017-07-12T07:22:36Z

Sorry for the long wait! I was busy with lots of other things ¯\_(ツ)_/¯

josephfrazier · 2017-07-14T16:53:33Z

No problem, thanks for the push! Would it be possible to automate the release process? I'm not exactly sure how it's currently handled.

fkling · 2017-07-20T01:54:00Z

Would it be possible to automate the release process?

Only if we

Get long term caching to work properly *
Forsake long term caching for the traditional "last-modified" approach

I followed different tutorials for this, but basically the way webpack is setup now results in tiny changes to files (module IDs are getting updated) without updating the chunk hash, which means the content changes but not the file name. This breaks the app for people. In these cases I have to bump the cache breaker.
Maybe we could even automate this, if someone wants to invest the time 😉

I'm thinking this might be less of a problem if we'd serve parser bundles completely separately from the website/UI itself ( #197).

I'm not exactly sure how it's currently handled.

Depending on what changed I may or may not run yarn build and yarn start and look over astexplorer locally.

Then I run ./scripts/build-site.sh to create a production build in a separate branch (gh-pages for historical reasons). I switch to the branch, and check the commit whether there have been changes to files without changing their names.
If yes I revert the commit, switch back to master, bump the cache breaker and run the script again.

Then I run ./scripts/deploy.sh server gh-pages which pushes the branch to the server.

On the server I have a post-receive hook that looks like

#!/bin/bash

set -e

repo=/data/astexplorer/repo
website=/data/astexplorer/website
server=/data/astexplorer/server
foldername=$(date +%Y-%m-%d_%H-%M-%S)

while read line; do
  case "$line" in
    *refs/heads/{{website_branch}}*)
      git clone --branch {{website_branch}} --depth 1 "file://$repo" $website/$foldername
      ln -f -n -s $foldername $website/latest
    ;;

    *refs/heads/{{server_branch}}*)
      git clone --branch {{server_branch}} --depth 1 "file://$repo" $server/$foldername
      cd $server/$foldername/server/
      # GIT_DIR is set which breaks package installation 
      # env -i creates a new empty environment
      env -i yarn
      ln -f -n -s $foldername $server/current
      echo "Restarting astexplorer server..."
      sudo systemctl restart astexplorer
    ;;
  esac
done

which checks out the latest version the website into a new repository (or updates and restarts the server).

Then I go to latest.astexplorer.net checking again whether everything is fine. If yes, I ssh into the server and update a symlink to point to the latest version.

Any suggestions for making this easier are appreciated 😄

josephfrazier · 2017-08-28T00:21:01Z

I'm not yet familiar enough with the webpack configuration to chime in on the caching issue, but it sounds like we could greatly simplify the process of reviewing and releasing pull requests:

With a few changes (one of which is #265), I was able to make it so that astexplorer can be deployed on Heroku (example). Heroku offers Review Apps, which makes it possible to deploy a pull request into an isolated environment (example). Similarly, deploying a new version after merging can be configured to happen automatically: https://devcenter.heroku.com/articles/github-integration.

How would you feel about switching to Heroku for hosting? It looks like the current server is set up to use CloudFlare (#69), but I think that can be done with Heroku as well: https://support.cloudflare.com/hc/en-us/articles/205893698-Configure-Cloudflare-and-Heroku-over-HTTPS

fkling · 2017-08-29T02:26:02Z

Thank your for investigating this @josephfrazier! I made some progress with a simplified version of #197. So if this works all out, PRs for updating parsers won't be needed anymore, and most of the problems should go away 😃

But I will keep the heroku solution in mind!

* Add typescript-eslint-parser I adapted this from the `babel-eslint` parser, as described in https://github.com/fkling/astexplorer/tree/263dc67a081abad45b74fc8a613bed6e9c00d5ff#how-to-add-a-new-parser This fixes fkling/astexplorer#218. * Remove extra arrow functions See fkling/astexplorer#242 (comment) * Use `displayName: ID` when they match See fkling/astexplorer#242 (comment) * typescript-eslint-parser: Handle null node.range See fkling/astexplorer#242 (comment) * typescript-eslint-parser: Remove babel-eslint leftovers See fkling/astexplorer#242 (comment) * typescript-eslint-parser: Add relevant settings from espree See fkling/astexplorer#242 (comment) * typescript-eslint-parser: Require range option to be enabled See fkling/astexplorer#242 (comment) * typescript-eslint-parser: Simplify nodeToRange() See fkling/astexplorer#242 (comment) * espree, typescript-eslint-parser: Show errors by default See fkling/astexplorer#242 (comment) * Remove unnecessary nodeToRange() implementation See fkling/astexplorer#242 (comment) * Add typescript-eslint-parser link to README * Transpile typescript-eslint-parser to fix build See fkling/astexplorer#242 (comment)

* Add typescript-eslint-parser I adapted this from the `babel-eslint` parser, as described in https://github.com/fkling/astexplorer/tree/2a5a0c18e266080c258e781c31012613a0e4a9e5#how-to-add-a-new-parser This fixes fkling/astexplorer#218. * Remove extra arrow functions See fkling/astexplorer#242 (comment) * Use `displayName: ID` when they match See fkling/astexplorer#242 (comment) * typescript-eslint-parser: Handle null node.range See fkling/astexplorer#242 (comment) * typescript-eslint-parser: Remove babel-eslint leftovers See fkling/astexplorer#242 (comment) * typescript-eslint-parser: Add relevant settings from espree See fkling/astexplorer#242 (comment) * typescript-eslint-parser: Require range option to be enabled See fkling/astexplorer#242 (comment) * typescript-eslint-parser: Simplify nodeToRange() See fkling/astexplorer#242 (comment) * espree, typescript-eslint-parser: Show errors by default See fkling/astexplorer#242 (comment) * Remove unnecessary nodeToRange() implementation See fkling/astexplorer#242 (comment) * Add typescript-eslint-parser link to README * Transpile typescript-eslint-parser to fix build See fkling/astexplorer#242 (comment)

Add typescript-eslint-parser

42f700f

I adapted this from the `babel-eslint` parser, as described in https://github.com/fkling/astexplorer/tree/b7ae2f06322c080313fdbea13501a19750d741ca#how-to-add-a-new-parser This fixes fkling#218.

RReverser reviewed Jun 19, 2017

View reviewed changes

josephfrazier added 3 commits June 19, 2017 10:16

Remove extra arrow functions

f04f3d3

See fkling#242 (comment)

Use displayName: ID when they match

38014ab

See fkling#242 (comment)

typescript-eslint-parser: Handle null node.range

4e4bfe9

See fkling#242 (comment)

RReverser reviewed Jun 19, 2017

View reviewed changes

josephfrazier added 2 commits June 19, 2017 18:46

typescript-eslint-parser: Remove babel-eslint leftovers

fca7ae9

See fkling#242 (comment)

typescript-eslint-parser: Add relevant settings from espree

8ee73e5

See fkling#242 (comment)

RReverser reviewed Jun 20, 2017

View reviewed changes

typescript-eslint-parser: Require range option to be enabled

4912cfb

See fkling#242 (comment)

RReverser reviewed Jun 20, 2017

View reviewed changes

typescript-eslint-parser: Simplify nodeToRange()

49f9ff9

See fkling#242 (comment)

RReverser reviewed Jun 21, 2017

View reviewed changes

espree, typescript-eslint-parser: Show errors by default

63e7c86

See fkling#242 (comment)

RReverser mentioned this pull request Jun 23, 2017

Add support for Scala #247

Merged

azz mentioned this pull request Jun 25, 2017

Add typescript-eslint-parser #249

Closed

azz reviewed Jun 25, 2017

View reviewed changes

josephfrazier added 4 commits June 27, 2017 15:34

Remove unnecessary nodeToRange() implementation

de37378

See fkling#242 (comment)

Merge branch 'master' into typescript-eslint-parser

08341ef

Add typescript-eslint-parser link to README

94ce628

Transpile typescript-eslint-parser to fix build

df20464

See fkling#242 (comment)

RReverser merged commit d4c9158 into fkling:master Jun 27, 2017

RReverser added the deploy pending Marks issues/PRs that are merged into master but are not deployed to any webserver label Jun 27, 2017

josephfrazier deleted the typescript-eslint-parser branch June 27, 2017 21:21

fkling added deployed to production Marks issues/PRs that are deployed to https://astexplorer.net and removed deploy pending Marks issues/PRs that are merged into master but are not deployed to any webserver labels Jul 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add typescript-eslint-parser #242

Add typescript-eslint-parser #242

josephfrazier commented Jun 18, 2017

RReverser Jun 19, 2017

RReverser Jun 19, 2017

RReverser Jun 19, 2017

josephfrazier commented Jun 19, 2017

RReverser Jun 19, 2017

RReverser commented Jun 19, 2017

josephfrazier commented Jun 19, 2017

RReverser Jun 20, 2017

josephfrazier Jun 20, 2017

RReverser Jun 20, 2017

josephfrazier Jun 20, 2017

RReverser Jun 20, 2017

RReverser Jun 20, 2017

josephfrazier Jun 20, 2017

RReverser Jun 20, 2017

josephfrazier Jun 20, 2017

fkling commented Jun 21, 2017

RReverser commented Jun 21, 2017

RReverser Jun 21, 2017

josephfrazier Jun 21, 2017 •

edited

Loading

RReverser Jun 21, 2017

RReverser commented Jun 23, 2017

azz Jun 25, 2017

josephfrazier commented Jun 27, 2017

RReverser commented Jun 27, 2017

josephfrazier commented Jun 27, 2017

fkling commented Jul 12, 2017 •

edited

Loading

josephfrazier commented Jul 14, 2017

fkling commented Jul 20, 2017

josephfrazier commented Aug 28, 2017

fkling commented Aug 29, 2017

Add typescript-eslint-parser #242

Add typescript-eslint-parser #242

Conversation

josephfrazier commented Jun 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephfrazier commented Jun 19, 2017

Choose a reason for hiding this comment

RReverser commented Jun 19, 2017

josephfrazier commented Jun 19, 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

fkling commented Jun 21, 2017

RReverser commented Jun 21, 2017

Choose a reason for hiding this comment

josephfrazier Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RReverser commented Jun 23, 2017

Choose a reason for hiding this comment

josephfrazier commented Jun 27, 2017

RReverser commented Jun 27, 2017

josephfrazier commented Jun 27, 2017

fkling commented Jul 12, 2017 • edited Loading

josephfrazier commented Jul 14, 2017

fkling commented Jul 20, 2017

josephfrazier commented Aug 28, 2017

fkling commented Aug 29, 2017

josephfrazier Jun 21, 2017 •

edited

Loading

fkling commented Jul 12, 2017 •

edited

Loading