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

Fix #3544, Allow multiple ancestors to be encapsulated with svelte class in case of ambiguity #3679

Merged
merged 13 commits into from
Oct 24, 2019

Conversation

jesseskinner
Copy link
Contributor

@jesseskinner jesseskinner commented Oct 9, 2019

I've fixed issue #3544 by allowing for multiple ancestors to match a selector, in the case that the ancestors have eg. dynamic classes and it's non-deterministic which of the ancestors will match.

I removed the filter that enforced only the first and last node to be encapsulated, so now multiple nodes are able to be encapsulated for a single selector-node pairing.

I didn't want to lose the optimization of skipping inner matches, so I had to change the logic so that it iterates over all blocks and ancestors until it finds the outermost non-global block that matches one or more ancestors. We include all the matching ancestors and then also include the child node.

apply_selector was originally written as a recursive function, but I chose to refactor out part of this into a block_might_apply_to_node function so that we're able to compare whether an ancestor node might match a selector block without doing it recursively.

Any feedback is very much welcome! This is my first time working with this code base so if there's anything I should have done differently, please let me know.

Thanks!

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases features are absent for a reason.
  • This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
  • Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

Tests

  • Run the tests tests with npm test or yarn test)

@jesseskinner
Copy link
Contributor Author

Two more notes I forgot to mention:

  1. I removed the Spreadsheet argument to apply_selector because it was unused.
  2. I was unable to get npm run lint to run properly, it might be broken in the latest version?

@jesseskinner jesseskinner changed the title Fix Issue #3544 Fix #3544, Allow multiple ancestors to be encapsulated with svelte class in case of ambiguity Oct 9, 2019
…kAppliesToNode to have three possible return values from block_might_apply_to_node
@jesseskinner
Copy link
Contributor Author

@Conduitry I've added an enum and removed the TypeError, let me know what you think.

@Conduitry Conduitry merged commit b6798e5 into sveltejs:master Oct 24, 2019
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.

3 participants