Skip to content
This repository has been archived by the owner on Aug 30, 2022. It is now read-only.

Extending walk this way steers people wrong #17

Closed
marijnh opened this issue Oct 16, 2018 · 2 comments
Closed

Extending walk this way steers people wrong #17

marijnh opened this issue Oct 16, 2018 · 2 comments

Comments

@marijnh
Copy link
Contributor

marijnh commented Oct 16, 2018

By returning a 'full' instance of the acorn-walk API, the library kind of suggests that the methods on that interface have been extended to support the new node type... but by default, if you don't pass them an explicit base walker, they'll use the base object that they closed over, which is the old, non-extended one. See acornjs/acorn#746.

I think a reasonable solution would be to create a module that just extends the base walker object, i.e. takes such an object and returns an extended version. That both works better with ES modules (less dynamic munging of exported interfaces) and steers people towards using the code correctly.

That'd be a breaking change, so if bumping the major version number is an issue, we could leave the old interface around and implement this in a separate file.

Would be happy to create a PR if this sounds reasonable.

@kesne
Copy link
Owner

kesne commented Nov 26, 2018

I'm not sure I understand the issue, can you demonstrate what this would end up looking like in code?

@kesne
Copy link
Owner

kesne commented Jun 4, 2019

Closing this. This is going to be added to Acorn, and can be filed there if it's still an issue.

@kesne kesne closed this as completed Jun 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants