Skip to content

Add method to lazily compute property source ranges#169

Merged
devongovett merged 2 commits intomasterfrom
property-location
May 10, 2022
Merged

Add method to lazily compute property source ranges#169
devongovett merged 2 commits intomasterfrom
property-location

Conversation

@devongovett
Copy link
Member

Closes #168

We don't store source locations for every property/value currently, because that would use a lot of additional memory, and it's rare to need source locations except in errors. So this adds a method to lazily compute the source locations of the keys/values of declarations within a style rule. Essentially, it performs a very lightweight additional parsing pass to first find the stored location of the parent style rule, and then finding the declaration by index from there.

This approach is somewhat error prone (e.g. if you modified the style sheet) so I'm not sure if it's a good idea or not. Alternatives would be to store source locations for everything, which I'd prefer not to do if we can help it, or somehow optionally store them externally (but I'm not sure how).

@g-plane
Copy link

g-plane commented May 2, 2022

somehow optionally store them externally

Is it possible that providing something like hooks when parsing then let downstream developers do something else, such as storing source locations?

@devongovett
Copy link
Member Author

Yeah possible, but how would you look them up? Making the nodes hashable or something wouldn't be enough because two nodes could be equal in different parts of the tree, but have different locations. We'd need to store at least some identifier in each node I guess.

@g-plane
Copy link

g-plane commented May 2, 2022

I see. Is it possible that calculating source locations for specific type of node in downstream code? For example, just for functions or colors.

a {
  border: 1px solid red;
                    ^^^
}

I'm not sure whether it can be implemented in downstream (to avoid making Parcel CSS itself complex and slow) or not.

@devongovett
Copy link
Member Author

Going to merge this for now. Will continue to think about ways to get more granular information in the future.

@devongovett devongovett merged commit b1afb80 into master May 10, 2022
@devongovett devongovett deleted the property-location branch May 10, 2022 04:43
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

Successfully merging this pull request may close these issues.

How can we get location of each AST node?

2 participants