Skip to content

Closes #9 #10

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.

Sign up for GitHub

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

Closed
wants to merge 7 commits into from
Closed

Closes #9 #10

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ea7f88f
move needsIndex to seprate file
peterbabic Dec 14, 2020
50ae64e
remove ciruclar dependency passing any via state
peterbabic Dec 14, 2020
2e09516
move needsIndex to any
peterbabic Dec 14, 2020
0f99c5d
make needsIndex private
peterbabic Dec 14, 2020
77431f0
make needsIndex private
peterbabic Dec 14, 2020
edf20d2
Update index.js
wooorm Dec 15, 2020
89f8735
move needsIndex into local scope
peterbabic Dec 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ var any = require('./lib/any')
var parse = require('./lib/parse')

function matches(selector, node) {
return Boolean(any(parse(selector), node, {one: true, shallow: true})[0])
return Boolean(any(parse(selector), node, {one: true, shallow: true, any: any})[0])
}

function select(selector, node) {
Expand Down
18 changes: 15 additions & 3 deletions lib/any.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
module.exports = match

var zwitch = require('zwitch')
var pseudo = require('./pseudo')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change can be reverted

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am getting quite lost here :sad:

What could be reverted, please?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were discussing two approaches.

  1. place needsIndex in any, remove it from pseudo, which doesn’t work.
  2. Pass any down

As 1 didn’t work, it can be reverted

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pardon me, but I am not seeing why it doesn't work. Could you please point me to the right direction?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You said here in the issue that changing needsIndex had no effect

Copy link
Contributor Author

@peterbabic peterbabic Dec 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have made another commit 89f8735 which I believe places needsIndex into even better place, by still not tripping any tests and with the circular dependency resolved.

If there is need to revert something, it has to be by your reasoning as a reviewer, otherwise I suspect I wasn't clear somewhere in the issue and your interpretation of what I have stated is different from what I have meant.

Please, provide way of moving this PR further, discarding the "this change could be reverted" suggestions in the process, if possible.

Copy link
Contributor Author

@peterbabic peterbabic Dec 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I might start getting what do you mean. To make things clear further, BOTH "approaches" need to be applied, not just the second one (with reverting the first one) to remove the cyclic dependency.

It is true that I said the first one does not work, meaning it reduced the dependency from direct (A -> B -> A) to indirect (A -> B -> C -> A, indirect cyclic dependencies tend to be easier to resolve) which was then resolved by the approach 2.

var test = require('./test')
var nest = require('./nest')

Expand Down Expand Up @@ -52,7 +51,8 @@ function rule(query, tree, state) {
scopeNodes: tree.type === 'root' ? tree.children : [tree],
iterator: iterator,
one: state.one,
shallow: state.shallow
shallow: state.shallow,
any: state.any
})
)

Expand All @@ -71,11 +71,23 @@ function rule(query, tree, state) {
}

function configure(query, state) {
var needsIndex = [
'first-child',
'first-of-type',
'last-child',
'last-of-type',
'nth-child',
'nth-last-child',
'nth-of-type',
'nth-last-of-type',
'only-child',
'only-of-type'
]
var pseudos = query.pseudos || []
var index = -1

while (++index < pseudos.length) {
if (pseudo.needsIndex.indexOf(pseudos[index].name) > -1) {
if (needsIndex.indexOf(pseudos[index].name) > -1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (needsIndex.indexOf(pseudos[index].name) > -1) {
if (pseudo.needsIndex.indexOf(pseudos[index].name) > -1) {

this change can be reverted

state.index = true
break
}
Expand Down
16 changes: 2 additions & 14 deletions lib/pseudo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,9 @@ module.exports = match
var zwitch = require('zwitch')
var not = require('not')
var convert = require('unist-util-is/convert')
var anything = require('./any')

var is = convert()

match.needsIndex = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change can be reverted

'first-child',
'first-of-type',
'last-child',
'last-of-type',
'nth-child',
'nth-last-child',
'nth-of-type',
'nth-last-of-type',
'only-child',
'only-of-type'
]

var handle = zwitch('name', {
unknown: unknownPseudo,
invalid: invalidPseudo,
Expand Down Expand Up @@ -61,6 +47,7 @@ function match(query, node, index, parent, state) {
function matches(query, node, index, parent, state) {
var shallow = state.shallow
var one = state.one
var anything = state.any
var result

state.one = true
Expand Down Expand Up @@ -160,6 +147,7 @@ function hasSelector(query, node, index, parent, state) {
var one = state.one
var scopeNodes = state.scopeNodes
var value = appendScope(query.value)
var anything = state.any
var result

state.shallow = false
Expand Down