ast-types

ast-types copied to clipboard

When I'm working on the ast-types and recast projects, I build them locally and then use npm link to test them. When I do this, my compiler seems to get confused between the output (.d.ts files) versus the source code (.ts files) because they are in the same folder. Although this can probably be fixed by tinkering with the compiler configuration, software projects normally have separate folders for their source code versus target files. (NPM/NodeJs did not originally follow this convention, but that's because without TypeScript, their the binaries and source code were the exact same file set.)

This PR is an experiment to update the ast-types project setup, moving source code into a conventional "src" subfolder, and the shipping output into a "lib" folder. This also enables us to simplify the .gitignore / .npmignore rulesets. It also eliminates the weird ts-emit-clean tool (that tries to reverse-engineer the tsconfig.json to pick out which files were probably written by the compiler); instead npm clean simply means deleting the output folder.

(If you like this approach, I'd be happy to open a similar PR to update the recast project.)

A question

When I tested this with recast, I ran into a problem that recast bypasses the main entry point and tries to import specific source files:

import { Omit } from "ast-types/types";

@benjamn is recast reaching into the internals of ast-types? If so, we can simply update recast's references.

Or are these entry points meant to be supported API contracts for library consumers? If so, could you provide a list of the specific files that are intended to be API contracts?

Thanks!

@benjamn I'm having the same issue again where the CI job fails for Node 6 and 7, but passes for all the other ones. How did you solve that last time?

@benjamn ping... :-)

@octogonz Thanks for working on this! Unfortunately, this would be a breaking change, since folks are supposed to be able to import, say, ast-types/def/babel.js in order to build a custom "fork" of the type system. Trust me when I say we would have switched to a src/lib project structure in #300 if we thought that was safe to do.

If we do end up making this change, perhaps we could retain the def directory, with stubs that re-export the lib/def/*.js modules. The lib directory itself (now src/lib) is a bit more problematic, but perhaps we can just use a different output directory name?

In any case, if we move forward here, I would like to group this breaking change together with some other improvements to TypeScript support. Specifically, the builders work very well in VSCode, but the namedTypes could be a bit more ergonomic, and I suspect those improvements will also be a breaking change that we should make at the same time as this change.

That said, if there are parts of this PR that you'd like to extract into separate PRs (that don't affect the project structure), they would probably get merged sooner!

Thanks for working on this! Unfortunately, this would be a breaking change, since folks are supposed to be able to import, say, ast-types/def/babel.js in order to build a custom "fork" of the type system.

@benjamn This is what I had alluded to above. Basically I was asking for an inventory (e.g. in README) of the file paths that are intended to be part of the public API contract. This PR could certainly preserve them. As you say we could have stubs that re-export the build outputs, or alternatively the toolchain could build directly into that folder (while still keeping the source files in a separate folder).

Whereas as things stand right now, no distinction is made between "internal" versus "public" files, so e.g. from a consumer's standpoint it's unclear which file paths are safe to import. I was hoping you could provide this inventory.

The lib directory itself (now src/lib) is a bit more problematic, but perhaps we can just use a different output directory name?

Sure. I've seen many projects that use project/lib used for compiler outputs and/or project/dist used for bundler outputs. But the names don't really matter to me. The important point was to separate toolchain inputs from outputs, since that's the cause of the actual compiler errors that motivated me to fix this.

In any case, if we move forward here, I would like to group this breaking change together with some other improvements to TypeScript support. Specifically, the builders work very well in VSCode, but the namedTypes could be a bit more ergonomic, and I suspect those improvements will also be a breaking change that we should make at the same time as this change.

FYI in my own project, I am unable to use recast's typings directly. Instead I input them from a wrapper like this:

rerecast.ts

import * as astTypes from 'ast-types/gen/nodes';
import * as astKinds from 'ast-types/gen/kinds';

// workaround for broken typings
import recastTypes = require('recast');
import recastModule from 'recast';
const recast: typeof recastModule = recastTypes as any;

const astChecks = recast.types.namedTypes;
const astBuilders = recast.types.builders;

export {
  astBuilders,
  astChecks,
  astKinds,
  astTypes,
  recastTypes,
  recast
}

I believe the trouble is caused by the "esModuleInterop": true setting in the tsconfig.json file. I was thinking about coming back with another PR to address that, since we aren't able to enable esModuleInterop in the project that's consuming recast. If this fix might break something, perhaps we could include it with the major version increment you mentioned.

@octogonz I know I've kept you waiting on this for a long time, but I'm happy to say your changes were relatively easy to rebase, and aligned well with similar changes I was thinking about making recently. Thanks so much for this work!

This is what I had alluded to above. Basically I was asking for an inventory (e.g. in README) of the file paths that are intended to be part of the public API contract. This PR could certainly preserve them. As you say we could have stubs that re-export the build outputs, or alternatively the toolchain could build directly into that folder (while still keeping the source files in a separate folder).

Whereas as things stand right now, no distinction is made between "internal" versus "public" files, so e.g. from a consumer's standpoint it's unclear which file paths are safe to import. I was hoping you could provide this inventory.

As an answer to this question, I added an "exports" section to package.json, which attempts to support existing imports from nested modules like ast-types/def/*.js by mapping them to ast-types/lib/def/*.js:

  "exports": {
    ".": "./lib/main.js",
    "./lib/*": "./lib/*.js",
    "./lib/*.js": "./lib/*.js",
    "./*": "./lib/*.js",
    "./*.js": "./lib/*.js"
  },

This will only help in versions of Node.js that understand the "exports" mapping, but I believe that includes Node.js v12.19.0 and later (docs). Older versions of Node.js should still be compatible with ast-types, but they may have to update nested ast-types/def/* imports with the actual path, ast-types/lib/def/* (technically a breaking change for them). Internally, nothing depends on this mapping, since the internals always use real relative paths to import other internal modules.

Despite all that effort to ensure an easy upgrade, I'm sure we've changed something between us that counts as a breaking change for someone, so I'm tempted to make this a major version bump instead of just a minor.

Since it's been so long, I understand if you are not especially interested in giving this a close review, but I appreciate your contribution and hope to get this released soon.

I've merged this into master so @eventualbuddha can try adding a def for TSInstantiationExpression to go with https://github.com/benjamn/recast/pull/1232. I will wait to publish a new version of ast-types to npm until we know everything is working well.

One thing that isn't working with this change: https://github.com/benjamn/ast-types/blob/1ea943179ed185fcca1bac606fe8705596935114/src/test/typescript.ts#L23-L29

The glob is no longer matching anything for, I think, two reasons:

1. the lib directory is where the file actually runs because it's being compiled to a separate directory now
2. the glob is looking for .js files instead of .ts files

I might be able to look at this, but it's a larger change than I intended to work on when looking at benjamn/recast#1232.

Thanks very much for noticing that. I can fix that and ping you when it's ready.

@benjamn I took a stab at it. It looks like there are a lot of TypeScript files that no longer parse properly. I'm not sure what to do about it 🤷

I have a draft PR up that is very close to restoring those tests: https://github.com/benjamn/ast-types/pull/886

Thank you for getting this released. Really cool! 😁