Run from AST Explorer #53

eventualbuddha · 2018-01-03T14:41:53Z

Begins work on #33.

@hzoo, what do you think of the proposed --remote-plugin option in this PR's README? I made it different from --plugin out of concern for security. I also think the implementation should probably make you confirm somehow before actually loading it.

TODO:

add --remote-plugin to help output
add a CLI test exercising --remote-plugin
document non-astexplorer.net URL loading
add failing tests for AstExplorerResolver and FileSystemResolver
make TestServer#protocol be http:
get review from @mcMickJuice
get sanity check from @fkling

hzoo · 2018-01-03T17:49:47Z

Awesome! Sounds good to me, we should add this to Babel maybe as well. Actually wish it would just be a part of babel-cli really to keep in sync (eventually)

eventualbuddha · 2018-01-04T15:28:10Z

I refactored the internals a little bit in order to support loading network plugins. It's partially broken at the moment but should be fairly easy to fix. The way I did it means we could just use e.g. --plugin https://astexplorer.net/#/gist/68827467161b95ee48048ff906fab6d8 and have that work. I wonder whether that's the right thing to do security-wise.

Also, since we're now using babel-register to transpile plugins I had to download the plugin code to a temporary file and then require that, which is okay but a little inelegant.

What do you think, @hzoo?

eventualbuddha · 2018-01-13T04:56:36Z

Okay, this now has both --plugin and --remote-plugin as originally described and is ready for review.

eventualbuddha · 2018-01-13T04:57:23Z

(I can't actually make either of you reviewers, but I'd like to have both @hzoo and @mcMickJuice take a look.)

mcMickJuice · 2018-01-13T15:14:38Z

For sure @eventualbuddha 👍

Just to make it easier on me, could you provide a high level overview of the changes you made? Looks like there were some significant changes

eventualbuddha · 2018-01-13T16:45:54Z

I’ll provide some inline comments. I had to rebase this branch after doing the prettier stuff and ended up just squashing everything together so I lost all my nicely-separated commits.

Also looks like I’m relying on URL which is not available in node v6.x. Seems like there are a few polyfill options that I can investigate.

eventualbuddha · 2018-01-13T16:47:47Z

README.md

 $ codemod --plugin transform-module-name \
  path/to/file.js \
  another/file.js
 ```

 This will re-write the files `path/to/file.js` and `another/file.js` by transforming them with the babel plugin `transform-module-name`. Multiple plugins may be specified, and multiple file or directories may be re-written at once.

+Plugins may also be loaded from [AST Explorer](https://astexplorer.net/) using `--remote-plugin`. This feature should only be used as a convenience to load code that you or someone you trust wrote. It will run with your full user privileges, so please exercise caution!


The way I wrote it you can actually load things from any URL that contains a babel plugin, but there’s special handling for astexplorer.net. I should probably document that here.

eventualbuddha · 2018-01-13T16:51:56Z

package.json

@@ -8,7 +8,7 @@
    "lint": "tslint --project .",
    "lint-fix": "tslint --project . --fix",
    "pretest": "yarn lint && tsc --project .",
-    "test": "mocha",
+    "test": "mocha 'test/**/*Test.js'",


I tried to avoid this, but I didn’t see a good way to do so. I wanted to make running from both the command line and editors (like WebStorm) work as expected with minimal configuration and duplication, and I can’t put globs into mocha.opts without forcing those files to always run.

eventualbuddha · 2018-01-13T16:52:29Z

package.json

    "typescript": "^2.2.1"
  },
  "dependencies": {
    "babel-core": "^6.26.0",
    "babel-preset-env": "^1.6.1",
    "babel-register": "^6.26.0",
    "glob": "^7.1.2",
+    "got": "^8.0.1",


got is added for fetching data from URLs.

eventualbuddha · 2018-01-13T16:52:48Z

package.json

    "mz": "^2.7.0",
    "recast": "^0.13.0",
-    "resolve": "^1.5.0"
+    "resolve": "^1.5.0",
+    "tmp": "^0.0.33"


tmp is added to create a temporary file for downloaded plugins.

eventualbuddha · 2018-01-13T16:55:27Z

src/Options.ts

@@ -4,6 +4,11 @@ import { hasMagic as hasGlob, sync as globSync } from 'glob';
 import { basename, extname, resolve } from 'path';
 import { sync as resolveSync } from 'resolve';
 import { PathPredicate } from './iterateSources';
+import PluginLoader from './PluginLoader';


Here’s where the big changes start. Rather than making loading the responsibility of Plugin, I created a PluginLoader class that takes a series of resolvers that can resolve things that are given on the command line (i.e. my-plugin from --plugin my-plugin) and turn them into file paths to be loaded. Once the file paths are resolved, PluginLoader does a require and extracts the babel plugin.

eventualbuddha · 2018-01-13T16:57:26Z

src/Options.ts

+
+  private remotePluginLoader = new PluginLoader([
+    new AstExplorerResolver(),
+    new NetworkResolver()


Eventually these lists might be configurable, but for now local plugins are loaded from either a direct file system reference or by package name and remote plugins are loaded either from astexplorer.net editor URLs or just a raw URL.

eventualbuddha · 2018-01-13T16:57:51Z

src/Options.ts

  private _pluginCache?: Array<Plugin>;

-  getPlugins(): Array<Plugin> {
+  async getPlugins(): Promise<Array<Plugin>> {


I made all these APIs async.

eventualbuddha · 2018-01-13T16:59:10Z

src/resolvers/AstExplorerResolver.ts

+ */
+export default class AstExplorerResolver extends NetworkResolver {
+  constructor(
+    private readonly baseURL: URL = new URL('https://astexplorer.net/')


This parameter is only used in tests so we can test against a local server instead of the real astexplorer.net.

eventualbuddha · 2018-01-13T17:02:30Z

test/cli/CLITest.ts

@@ -5,7 +5,7 @@ import { basename, dirname, join } from 'path';
 import { sync as rimraf } from 'rimraf';

 function plugin(name: string): string {
-  return join(__dirname, `fixtures/plugin/${name}.js`);
+  return join(__dirname, `../fixtures/plugin/${name}.js`);


Most of these sort of changes were just moving CLI tests into test/cli and everything else into test/unit to enable running the unit tests by themselves more quickly. That’s somewhat nullified by the required tsc -p . step, but such is life. I’m open to suggestions for ways to better speed up the tests (@mcMickJuice mentioned Jest).

eventualbuddha · 2018-01-13T17:03:07Z

test/mocha.opts

@@ -0,0 +1 @@
+--require source-map-support/register


We were already building sourcemaps, but we need this to make stack traces actually use them.

eventualbuddha · 2018-01-13T17:04:12Z

test/unit/TestServer.ts

+export class RealTestServer {
+  private server: Server;
+
+  readonly protocol: string = 'http';


I think this actually should be http:.

eventualbuddha · 2018-01-13T17:05:37Z

test/unit/TestServer.ts

+  res: ServerResponse
+) => void;
+
+export class RealTestServer {


I didn’t look very hard for another package that implements something like this. I found mocking libraries for http but those always felt messy to me, and so I chose to have a real local server that could be hit for testing.

eventualbuddha · 2018-01-13T17:07:16Z

test/unit/resolvers/AstExplorerResolverTest.ts

+import AstExplorerResolver from '../../../src/resolvers/AstExplorerResolver';
+import { startServer } from '../TestServer';
+
+describe('AstExplorerResolver', function() {


I should probably add some failure cases here.

eventualbuddha · 2018-01-13T17:07:35Z

test/unit/resolvers/FileSystemResolverTest.ts

+import { join } from 'path';
+import FileSystemResolver from '../../../src/resolvers/FileSystemResolver';
+
+describe('FileSystemResolver', function() {


I should probably add some failure cases here.

eventualbuddha · 2018-01-13T17:08:48Z

tslint.json

@@ -1,37 +1,21 @@
 {
-	"rules": {
+  "extends": [
+    "tslint-config-prettier"


I ended up having to do what you were suggesting on #56 to make sure tslint and prettier didn’t conflict, @mcMickJuice.

eventualbuddha · 2018-01-13T17:11:02Z

Okay, hopefully that’s enough context for you to take a look @mcMickJuice. Also, @fkling, does this look sane from your perspective? I don’t know whether there’s some obvious cases where using astexplorer.net in this way will fail.

mcMickJuice · 2018-01-13T20:15:57Z

src/Options.ts

+        })
+      );
+
+      this._pluginCache = [...(await localPlugins), ...(await remotePlugins)];


isnt async/await the best?

mcMickJuice

I think the approach is solid! I haven't run the code yet but just looking through changes and making minor comments and tweaks, it looks awesome.

mcMickJuice · 2018-01-13T20:39:25Z

src/resolvers/FileSystemResolver.ts

+ */
+export default class FileSystemResolver implements Resolver {
+  constructor(
+    private readonly optionalExtensions: Set<string> = DEFAULT_PLUGIN_EXTENSIONS


How would the user pass in additional extensions?

They wouldn't at this point. We might be able to use this for #57.

mcMickJuice · 2018-01-13T20:39:43Z

src/resolvers/FileSystemResolver.ts

+import { resolve } from 'path';
+import Resolver from './Resolver';
+
+const DEFAULT_PLUGIN_EXTENSIONS = new Set(['.js']);


should we support .ts by default as well?

Maybe? But we don't actually support them out of the box.

Good point. then disregard my comment

mcMickJuice · 2018-01-13T20:40:42Z

src/resolvers/NetworkResolver.ts

+      throw new NetworkLoadError(response);
+    }
+
+    let filename = tmp();


We create temporary files in the CLITests. Maybe make a note to use this package for temporary tests files as well?

Good point. I added a comment.

mcMickJuice · 2018-01-13T20:41:45Z

src/index.ts

@@ -13,6 +13,7 @@ ${$0} [OPTIONS] [PATH … | --stdio]

 OPTIONS
  -p, --plugin PLUGIN               Transform sources with PLUGIN (allows multiple).
+      ---remote-plugin URL          Fetch a plugin from URL (allows multiple).


remove extra -

mcMickJuice · 2018-01-13T20:44:35Z

test/unit/TestServer.ts

+  res: ServerResponse
+) => void;
+
+export class RealTestServer {


mcMickJuice · 2018-01-13T20:54:55Z

src/resolvers/FileSystemResolver.ts

+  ) {}
+
+  private *enumerateCandidateSources(source: string): IterableIterator<string> {
+    yield resolve(source);


I think I misunderstood the flow in this function. optionalExtensions is used here so that the user can provide a path path/to/filename.js or path/to/filename, where currently if extension is omitted, we will try to resolve using this extension array. Is that correct?

Yes, that's right.

eventualbuddha · 2018-01-13T20:58:46Z

package.json

@@ -17,6 +17,10 @@
    "*.ts": [
      "prettier --parser typescript --write",
      "git add"
+    ],
+    "*.md": [


I remembered that prettier can reformat Markdown, so since I was editing the README anyway I figured I'd add that here.

eventualbuddha · 2018-01-13T20:59:22Z

package.json

-    "resolve": "^1.5.0"
+    "resolve": "^1.5.0",
+    "tmp": "^0.0.33",
+    "whatwg-url": "^6.4.0"


I'm using this instead of the builtin url since node v6.x doesn't have url.URL.

Tests pass now!

eventualbuddha · 2018-01-13T21:00:58Z

src/resolvers/AstExplorerResolver.ts

+import { URL } from 'whatwg-url';
+import NetworkResolver from './NetworkResolver';
+
+const EDITOR_HASH_PATTERN = /^#\/gist\/(\w+)(?:\/(\w+))?$/;


I just changed this to not include the hostname, since we're supposed to be able to customize it in the constructor argument.

fkling · 2018-01-14T01:28:36Z

Seems fine from my side. You could consider using the github API directly instead of going through astexplorer (this is what astexplorer itself is doing), but probably not a big deal.

Note that the structure of the data in the gists could change in the future (in other words, I can't promise that I will remember to notify you about such changes, but I'll try 😉 )

eventualbuddha · 2018-01-14T01:38:20Z

Thanks @fkling! I'll probably leave it as-is for now. In the future it might be nice to allow more flexibly pulling files out of gists, something like this:

$ codemod --remote-plugin gist:68827467161b95ee48048ff906fab6d8#transform.js

Exactly what syntax to use for that I'm not sure.

eventualbuddha · 2018-01-14T20:43:45Z

Alright, this is now available in v1.4.0!

eventualbuddha force-pushed the run-from-astexplorer branch from c1085cc to 87b9952 Compare January 5, 2018 15:38

eventualbuddha force-pushed the run-from-astexplorer branch 2 times, most recently from 7184613 to 86558fa Compare January 13, 2018 04:55

eventualbuddha changed the title ~~[WIP] Run from AST Explorer~~ Run from AST Explorer Jan 13, 2018

eventualbuddha commented Jan 13, 2018

View reviewed changes

eventualbuddha force-pushed the run-from-astexplorer branch from 86558fa to 80aef03 Compare January 13, 2018 17:50

mcMickJuice reviewed Jan 13, 2018

View reviewed changes

eventualbuddha force-pushed the run-from-astexplorer branch from 80aef03 to b9eccd3 Compare January 13, 2018 20:56

eventualbuddha commented Jan 13, 2018

View reviewed changes

feat(plugins): allow loading from remote sources

05131ea

eventualbuddha force-pushed the run-from-astexplorer branch from b9eccd3 to 05131ea Compare January 13, 2018 22:48

eventualbuddha merged commit 706ebb1 into master Jan 14, 2018

eventualbuddha deleted the run-from-astexplorer branch January 14, 2018 20:38

hzoo mentioned this pull request Jan 15, 2018

Babel-core: make it a peerDep #41

Closed

Run from AST Explorer #53

Run from AST Explorer #53

Conversation

eventualbuddha commented Jan 3, 2018 • edited Loading

hzoo commented Jan 3, 2018

eventualbuddha commented Jan 4, 2018

eventualbuddha commented Jan 13, 2018

eventualbuddha commented Jan 13, 2018

mcMickJuice commented Jan 13, 2018 • edited Loading

eventualbuddha commented Jan 13, 2018

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

eventualbuddha commented Jan 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcMickJuice 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

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

fkling commented Jan 14, 2018

eventualbuddha commented Jan 14, 2018

eventualbuddha commented Jan 14, 2018

eventualbuddha commented Jan 3, 2018 •

edited

Loading

mcMickJuice commented Jan 13, 2018 •

edited

Loading