[estree] Use null for optional fields in estree

[ivogabe]

• Make your PR against the master branch.
• Use a meaningful title for the pull request. Include the name of the package modified.
• Test the change in your own code. (Compile and run.)
• Follow the advice from the readme.
• Avoid common mistakes.
• Run tsc without errors.
• Run npm run lint package-name if a tslint.json is present.

Select one of these and delete the others:

If changing an existing definition:

• Provide a URL to documentation or source code which provides context for the suggested changes: https://github.com/estree/estree/blob/master/es5.md
• Increase the version number in the header if appropriate.
• If you are making substantial changes, consider adding a tslint.json containing { "extends": "../tslint.json" }.

Optional fields are currently marked with ?, meaning that the field may be undefined, but not null. Though the specification at https://github.com/estree/estree/blob/master/es5.md#node-objects uses | null for optional fields (and they really mean null, not null | undefined). This PR fixes this.

One exception: async and generator are marked optional and I kept it that way, since they are optional since they were added in a language extension. So these properties can be absent when using a tool that works with ES5.

@RReverser Do you agree?

[dt-bot]

estree/flow.d.ts

to author (@RReverser). Could you review this PR?
👍 or 👎?

Checklist

• pass the Travis CI test?

---

estree/index.d.ts

to author (@RReverser). Could you review this PR?
👍 or 👎?

Checklist

• pass the Travis CI test?

[RReverser]

While I'm on board with allowing explicit null, I'm not sure about the other change. In the spec, | null marks anything that is optional, and producers might actually produce AST nodes without such fields at all, and consumers will accept either.

async and generator are just a few of such cases - for example, the node with missing loc is still totally valid and can be produced by e.g. Acorn when location tracking is disabled. Same for BreakStatement / ContinueStatement nodes without label or VariableDeclarator without init etc.

While we potentially might go through each type and make decisions on whether it's ok to have such field optional or we want explicit null, I'm not sure it would be worth it (especially given that this is a breaking change for existing TS code) and rather would suggest for now to change declarations to prop?: sometype | null to accept both omitted properties and explicit null variants.

[mhegazy]

So @RReverser and @ivogabe is this PR ready to go or not?

[RReverser]

@mhegazy It's not.

[ivogabe]

The spec says the following:

The loc field represents the source location information of the node. If the node contains no information about the source location, the field is null; otherwise it is an object consisting of [..]

Though I understand that the spec could be interpreted less strictly. So I've looked at implementations in various projects. It appears that parsers (Babylon, Esprima, Acorn) do explicitly set null for missing fields (for instance for a BreakStatement). However Acorn and Esprima don't set loc to null, meaning that loc defaults to undefined.

Some consumers check for null with a truthiness check or == null, and those will work fine with undefined. Though eslint and babili only check for null and those tools will thus break when passing undefined instead.

So I'm not sure what the best approach would be. I'd like to have it as strict as possible; maybe mark only loc as optional?

[RReverser]

@ivogabe ESLint and Babili can allow themselves to perform stricter checks as they're getting information directly from parser, but in general for the format explicit null is not guaranteed (as you've seen from other examples), because any transform can create node without them and still work.

Problem with marking fields as non-optional and with type of SomeNode | null is that TypeScript code won't be able to correctly consume previously accepted code from all these various JS-based transformers, which leads to inconsistency between runtime (observed) and declared time. This kind of behavior is worse that having slightly broader type definition IMO.

[ivogabe]

Do you have concrete examples of transforms which do set undefined or use delete to delete an optional field? Though one could argue that those transforms are not spec compliant, so I'm not yet convinced that these fields should be marked with a ?.

[RReverser]

@ivogabe It's not about setting undefined or deleting, but about creating new nodes - transforms usually don't care about setting such optional ones to null explicitly. I myself also wrote quite a bit of transforms and worked on spec itself, treating such fields as optional in both cases.

[RReverser]

FWIW, this is also how Babel (which uses fork of ESTree, but still pretty close) defines such fields: https://github.com/babel/babel/blob/d33d02359474296402b1577ef53f20d94e9085c4/lib/types.js#L75-L78

[ivogabe]

Ok, I've marked them optional again in af72627. I think that the spec could be clarified on this topic, so I opened estree/estree#158. Are the changes here ok?

[paulvanbrenk]

@RReverser good to go now?

[RReverser]

@ivogabe Can you please change some of tests back so that they would cover both optional and explicit null syntaxes?

[ghost]

We've recently updated the repository structure.
All packages are now in a types subdirectory.
Please rebase this pull request onto the latest master branch.

[RReverser]

Repository change structure just in time :(

[RyanCavanaugh]

@ivogabe last ping on fixing merge conflicts

[ivogabe]

Sorry for the long delay, will update this today