Closes #9 #10

peterbabic · 2020-12-14T20:18:15Z

This PR fixes #9

It is not a bug fix, but rather a code quality improvement. It's purpose is to stop JS bundler complaining about the detected circular dependency.

It does not include any new tests, because it would require testing for circular dependency, not sure what the right approach would be right now. The issue thread shows dependency graphs and a clear resolve of the cyclic dependency that existed before PR.

Note that possible state (context) pollution introduced by passing any down the road from the index.js

Tries to remove cyclical dependency any.js -> psudo.js addressing [syntax-tree#9](syntax-tree#9)

wooorm

thanks for taking a stab at this!

index.js

lib/any.js

Co-authored-by: Titus <tituswormer@gmail.com>

wooorm

The recursiveness seems to only be needed in matches (x:matches(:link)) and hasSelector (x:has(:link)). I’m assuming those two aren’t tested with pseudos in the select or selectAll cases

wooorm · 2020-12-15T09:47:45Z

lib/pseudo.js


 var is = convert()

-match.needsIndex = [


this change can be reverted

wooorm · 2020-12-15T09:48:04Z

lib/any.js

@@ -75,7 +88,7 @@ function configure(query, state) {
  var index = -1

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


Suggested change

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

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

this change can be reverted

wooorm · 2020-12-15T09:48:12Z

lib/any.js

 var zwitch = require('zwitch')
-var pseudo = require('./pseudo')


this change can be reverted

I am getting quite lost here :sad:

What could be reverted, please?

We were discussing two approaches.

place needsIndex in any, remove it from pseudo, which doesn’t work.

Pass any down

As 1 didn’t work, it can be reverted

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

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

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.

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.

wooorm · 2020-12-15T09:48:26Z

lib/any.js

+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'
+]
+


Suggested change

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'

]

this change can be reverted

Configure function is the sole consumer of the needsIndex array, it shoould then be in it's local scope. This change further improves code quiality.

wooorm · 2020-12-15T10:52:01Z

I don’t see why the changes previously applied for approach 1 are needed. Moving any to something that is passed around removes the recursive dep.

Could you make the changes in this PR as small as possible? Only the change that any is passed around, not the needsIndex changes.

And could you pass any: any in index.js to the two other functions too?

peterbabic · 2020-12-15T10:55:29Z

Or in even different words, there are TWO cyclic dependencies to begin with, the first approach removes the direct one (yellow), the second approach removes the indirect one (red)

wooorm · 2020-12-15T10:57:06Z

By passing any down to all files, that is removed though?

wooorm · 2020-12-15T11:09:26Z

Hold on, let me try and do it myself. I’m sorry that I can’t seem to make my suggestion clear.

peterbabic · 2020-12-15T11:10:13Z

I am sorry, you are right. Just by applying the approach 2 the cyclic dependency is resolved.

However by passing any: any into contextx into select and selectAll does not seem to affect anything

wooorm · 2020-12-15T11:13:26Z

I described select and selectAll here: #10 (comment)

peterbabic · 2020-12-15T11:20:42Z

Well, to my best efforts:

Approach 2, minimal code needed for tests passing and cyclic dependency removed
peterbabic@375cdfe
Approach 2 + passing any also into select and selectAll, not just into matches
peterbabic@e455227

These two could be squashed if needed, but TBH I am not quite there yet about the #10 (comment)

wooorm · 2020-12-15T11:22:15Z

I’m already working on that approach, thanks, will push soon (but install graphviz for marge broke my computer 🥺)

peterbabic added 3 commits December 14, 2020 19:14

move needsIndex to seprate file

ea7f88f

Tries to remove cyclical dependency any.js -> psudo.js addressing [syntax-tree#9](syntax-tree#9)

remove ciruclar dependency passing any via state

50ae64e

move needsIndex to any

2e09516

peterbabic marked this pull request as ready for review December 14, 2020 20:24

wooorm reviewed Dec 14, 2020

View reviewed changes

index.js Outdated Show resolved Hide resolved

lib/any.js Outdated Show resolved Hide resolved

lib/any.js Outdated Show resolved Hide resolved

peterbabic and others added 2 commits December 14, 2020 21:35

make needsIndex private

0f99c5d

Co-authored-by: Titus <tituswormer@gmail.com>

make needsIndex private

77431f0

Co-authored-by: Titus <tituswormer@gmail.com>

peterbabic requested a review from wooorm December 14, 2020 20:45

Update index.js

edf20d2

wooorm reviewed Dec 15, 2020

View reviewed changes

move needsIndex into local scope

89f8735

Configure function is the sole consumer of the needsIndex array, it shoould then be in it's local scope. This change further improves code quiality.

wooorm closed this in 5da284c Dec 15, 2020

wooorm added ⛵️ status/released 🏡 area/internal This affects the hidden internals 👶 semver/patch This is a backwards-compatible fix 🦋 type/enhancement This is great to have labels Dec 15, 2020

wooorm added the 💪 phase/solved Post is done label Apr 12, 2021

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

		var zwitch = require('zwitch')
		var pseudo = require('./pseudo')

Uh oh!

Closes #9 #10

Closes #9 #10

Uh oh!

Conversation

peterbabic commented Dec 14, 2020

Uh oh!

wooorm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wooorm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterbabic Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterbabic Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wooorm commented Dec 15, 2020

Uh oh!

peterbabic commented Dec 15, 2020

Uh oh!

wooorm commented Dec 15, 2020

Uh oh!

wooorm commented Dec 15, 2020

Uh oh!

peterbabic commented Dec 15, 2020

Uh oh!

wooorm commented Dec 15, 2020

Uh oh!

peterbabic commented Dec 15, 2020

Uh oh!

wooorm commented Dec 15, 2020

Uh oh!

Uh oh!

wooorm left a comment •

edited

Loading

peterbabic Dec 15, 2020 •

edited

Loading

peterbabic Dec 15, 2020 •

edited

Loading