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

Breaking changes in 6.0 #656

Closed
marijnh opened this issue Jan 19, 2018 · 15 comments
Closed

Breaking changes in 6.0 #656

marijnh opened this issue Jan 19, 2018 · 15 comments

Comments

@marijnh
Copy link
Member

marijnh commented Jan 19, 2018

Whether we want to move to 6.0 on the short term is up for debate, but there's a very nice performance increase in #655 that'd break internal API compatibility, so it might be worthwhile.

So while we're at it, I'd also like to discuss another thing I feel we should change at some point. At the moment, plugins

  • are globally registered under a unique name, and

  • mutate fully constructed instances of the parser, extending methods through a kludgy extend method

Neither of these is great. The first is, now that everyone is using module systems, somewhat obsolete, and we might as well do a thing where we pass an array of objects or functions as plugins.

The second is rather slow and awkward, and with class syntax, I think we could just make plugins mixins like this:

function myPlugin(Parser) {
  return new class extends Parser {
    parseExprAtom(...) {
       if (...) return something; else return super(...);
    }
  };
}

To apply an array of these, you'd just run the base class through all of them, extending it at each point. That's easier to write and I expect it is faster. It'd definitely be faster if we make some other API changes to allow people to construct the extended class once, and then reuse it across parses, so that inline caches aren't constantly clobbered because method identities keep changing.

Any other ideas?

@marijnh
Copy link
Member Author

marijnh commented Jan 19, 2018

Also, we should seriously consider layering our interface so that regular client code doesn't need to constantly upgrade to a new major version although the external interface didn't change. This might look something like this:

  • There's a package acorn-internal that plugins depend on. This constantly has its major version bumped as we make breaking changes to the internal interface.

  • Then there's acorn, which depends on acorn-internal, but only exports the regular, client code interface. It can be much more stable, since that interface has much less reason to change.

How does that sound?

This does leave the periodic bumping of the default ecmaVersion option as a source of breaking changes to the client code API. Since new versions tend to be almost entirely backwards-compatible, maybe we should document this to always default to the latest version that Acorn can parse, and stop calling new defaults a breaking change?

@marijnh
Copy link
Member Author

marijnh commented Jan 19, 2018

cc @adrianheine @RReverser

@RReverser
Copy link
Member

Why don't we make just few public methods that plugins actually need to use (in my experience, there is really not many of these) and hide all the others unexported in a major bump? This would allow to make internal changes more freely whenever needed and also to finally document API for plugins (as documenting every single current method on prototype as it is now is, admittedly, quite hard and of questionable usability in face of frequent small changes).

@marijnh
Copy link
Member Author

marijnh commented Jan 19, 2018

in my experience, there is really not many of these

I haven't looked at a lot of plugins, but I expect that this would seriously limit the things you could do with them. So far, the promise of plugins is that you can override any syntactic construct, even extend the tokenizer, and implement your extensions using the utility functions that the parser itself uses. There's a lot of methods that are useful in that context.

A quick look through some of the existing plugins yields this list of parser properties used:

canInsertSemicolon, checkLVal, checkPatternErrors, checkPatternExport, containsEsc, eat, eatContextual, expect, expectContextual, finishNode, finishToken, inModule, input, next, options, parseClassMethod, parseExpression, parseExpressionStatement, parseIdent, parseMaybeAssign, parsePropertyName, parsePropertyValue, parseSpread, pos, raise, raiseRecoverable, semicolon, start, startLoc, startNode, startNodeAt, toAssignable, type, unexpected, value

And there's a whole lot more that I can imagine a valid use for. So unless we find some very different form of abstracting the parser, I don't think a narrow interface is going to cut it.

@adrianheine
Copy link
Member

I'd really like this acorn-internal idea! In any way I'll probably find a few breaking changes I'd like to do.

@adrianheine
Copy link
Member

Just a quick list:

  • Drop value for (some?) literals (probably add an option)
  • Pass node to parseBlock
  • Always pass node from parseExprAtom
  • Drop classBody param from parseClassMember and parseClassMethod
  • Rename parseClassMember to parseClassElement since that's the term in the spec (yeah, I know I just introduced that)

@marijnh
Copy link
Member Author

marijnh commented Jan 26, 2018

If we're going to rename stuff to match the spec, I think there's a lot more—the method names don't follow the names in the grammar at all—but I'm not sure how much of a problem that is.

@adrianheine
Copy link
Member

It's not much of a problem, I think, so I don't want to change all names, it's just that I introduced that one recently and it's such a blatantly bad choice. I would also want to drop support (at least for developing) for node < 4. How do you feel about walk and loose? Should they stay in the same repo?

@marijnh
Copy link
Member Author

marijnh commented Feb 7, 2018

How do you feel about walk and loose? Should they stay in the same repo?

Good point. If we're going to break things, they might as well move into separate NPM packages. I would keep them in the same repo, to make it easier to keep the loose parser working as features are added to the regular parser.

(Then again, I'm not developing Tern anymore, so my commitment to the loose parser isn't all that great. Would be interesting to know how many people are still using that. I guess publishing it as a separate NPM package is a good first step for that.)

@adrianheine
Copy link
Member

Apparently, dropping ScopeBody and ScopeExpression in walk is something you wanted to do in a breaking change.

I'm a bit surprised, but I agree that putting some or all of the acorn modules in a shared repository makes sense.

@marijnh
Copy link
Member Author

marijnh commented Sep 10, 2018

On second thought, don't think treating default ecmaVersion bumps as non-breaking changes is a reasonable policy (you might, on upgrade, suddenly get new AST nodes that you're not able to handle).

I've, for now, decided against the separate plugin API package. Since we're developing at such a glacial pace, and ecmaVersion bumps will require new major versions every now and then anyway, they seem to introduce more complexity than they justify.

Other than that, I've starting making the breaking change that I want in 6.0 on master. I'll continue working on the plugin interface tomorrow, and plan to release 6.0 later this week. Let me know if you have doubts about any of the changes.

@marijnh
Copy link
Member Author

marijnh commented Sep 11, 2018

All right, I have things more or less in the way I want them now. The submodules live under acorn/, acorn-loose/, and acorn-walk/, and will be npm published from there. I've ported the most popular plugins to the new interface and submitted pull requests there. I'd like to go ahead and release later this week.

@RReverser and @adrianheine If you have time to take a look in the coming days and tell me what you think, that'd be appreciated.

@marijnh
Copy link
Member Author

marijnh commented Sep 13, 2018

(I intend to release tomorrow, so if you have feedback, let's hear it.)

@adrianheine
Copy link
Member

I'm ok with this and will do our plugins after release.

@marijnh
Copy link
Member Author

marijnh commented Sep 14, 2018

Published version 6.

@marijnh marijnh closed this as completed Sep 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants