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

Use treeAdapter to attach location information #189

Closed
inikulin opened this issue Mar 25, 2017 · 3 comments
Closed

Use treeAdapter to attach location information #189

inikulin opened this issue Mar 25, 2017 · 3 comments

Comments

@inikulin
Copy link
Owner

Currently location always implicitly stored as __location property. It breaks paradigm of not enforcing tree format in parse5. It makes sense to introduce treeAdapter.setNodeLocationInfo method that will give implementers opportunity to decide how to store it.
Another thing that bothered for a long time is the name of the __location property. Since where will be a major version bump due to the changes in the location info layout required for error reporting feature I guess it's a good chance to rename it to location.

@diervo
Copy link
Contributor

diervo commented Mar 26, 2017

@inikulin Are you planning to use the same paradigm to inject the error types?

I like an imperative API much better than the simple AOP hooks that exists today for location, which would be very hard to reason once you have more than one injection.

@inikulin
Copy link
Owner Author

Location will still be implemented via hooks. For errors there will be combination of imperative and AOP. I don't like it either, but we can't use imperative approach for this feature since it will harm performance in basic case. Maybe, maybe I'll revisit this topic one day being fed up with current approach, but meanwhile it doesn't bother me that much.

@inikulin inikulin added this to the 5.0 milestone Apr 16, 2018
@inikulin
Copy link
Owner Author

Tree adapter will need to implement two methods:

  • setNodeSourceCodeLocation
  • getNodeSourceCodeLocation

__location property will be renamed to sourceCodeLocation.

Both changes are breaking.

inikulin added a commit that referenced this issue May 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants