Skip to content

node first, test second #6

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

Closed
wooorm opened this issue Aug 17, 2018 · 10 comments · Fixed by #8
Closed

node first, test second #6

wooorm opened this issue Aug 17, 2018 · 10 comments · Fixed by #8
Labels
🗄 area/interface This affects the public interface 💪 phase/solved Post is done 🧑 semver/major This is a change 🦋 type/enhancement This is great to have

Comments

@wooorm
Copy link
Member

wooorm commented Aug 17, 2018

Unist utilities operate on nodes. Makes more sense passing the Node first. Then the test. Optionally other stuff.

@wooorm
Copy link
Member Author

wooorm commented May 29, 2019

@ChristianMurphy What do you think of this?

Example use
var is = require('unist-util-is')

var node = {type: 'strong'}
var parent = {type: 'paragraph', children: [node]}

is() // => false
is({children: []}) // => false
is(node) // => true
is(node, 'strong') // => true
is(node, 'emphasis') // => false

is(node, node) // => true
is(parent, {type: 'paragraph'}) // => true
is(parent, {type: 'strong'}) // => false

is(node, test) // => false
is(node, test, 4, parent) // => false
is(node, test, 5, parent) // => true

function test(node, n) {
  return n === 5
}

Reasoning:

  • Aligns with hast-util-is-element
  • Typically more readable (cases where there is a node and a test)
  • But it is a bit weird node, index, parent are not passed next to each other

Downside:

  • Will break people directly using unist-util-is, although most people use it through something else, like unist-util-visit, which ca be updated to use convert anyway.

@ChristianMurphy
Copy link
Member

An interesting idea 🤔
Thinking about the structure, it seems to imply that one node will be tested in several different ways.
Not a use case I have had yet, but it makes sense.

It seems like it may overlap a bit with https://github.com/tc39/proposal-pattern-matching


Aligns with hast-util-is-element

It makes sense to bring these in alignment.

Typically more readable (cases where there is a node and a test)

I've typically created the test as it's own named function. To me:

is(test, node);
is(node, test);

are pretty close in readability. 🤷‍♂️

I can see where a more complex condition, put inline could make putting the test after more attractive.

is((n) => {
  /* some
   * complex
   * conditionals */
  },
  node
)

is(node, (n) => {
  /* some
   * complex
   * conditionals */
  }
)

Will break people directly using unist-util-is

There may be an opportunity to create a code mod to help people upgrade.
https://github.com/facebook/jscodeshift

E.G. from the react community https://github.com/reactjs/react-codemod

@ChristianMurphy
Copy link
Member

Another thing that gives me pause, there are 62 packages that would need to be updated https://github.com/syntax-tree/unist-util-is/network/dependents?dependent_type=PACKAGE

Which is partly why I'd be interested in a code mod.

@wooorm
Copy link
Member Author

wooorm commented May 30, 2019

I made a list of the 13 projects under syntax-tree that are using this, and they can almost all be moved to convert anyway, which I’ll do after this!
Most of the other packages seem to either be a) not used in the last year, or b) under the rest of the collective. So I’m not toooo worried about this.

I think code mods are interesting, but wonder how much time would go into creating and managing them? Makes sense for big user-facing stuff like React of course, but as the unified collective (except for MDX) is so low level and changes are infrequent...

@ChristianMurphy
Copy link
Member

Makes sense for big user-facing stuff like React of course, but as the unified collective (except for MDX) is so low level and changes are infrequent

Makes sense

made a list of the 13 projects under syntax-tree that are using this, and they can almost all be moved to convert anyway, which I’ll do after this!

Semi-related, would there be interest in leveraging https://dependabot.com or https://renovatebot.com to help automate part of the process?

@wooorm wooorm closed this as completed in #8 May 31, 2019
wooorm added a commit that referenced this issue May 31, 2019
Closes GH-6.
Closes GH-8.

Reviewed-by: Christian Murphy <[email protected]>
@wooorm
Copy link
Member Author

wooorm commented May 31, 2019

Semi-related, would there be interest in leveraging https://dependabot.com or https://renovatebot.com to help automate part of the process?

Yes! But it has to not be annoying! I switched to use greenkeeper everywhere, before, but that made PRs for everything, all the time. I’m maintaining about 400 projects and if I update remark-cli I don’t want to deal with 400 PRs 😅

I believe renovatebot (and maybe dependabot, or even greenkeeper now) have better support from the setup I saw you use, such as sending PRs every X span of time?

Mostly I think I’d be interest in something that only checks dependencies, not dev-dependencies. Is that possible?

@ChristianMurphy
Copy link
Member

have better support from the setup I saw you use, such as sending PRs every X span of time?
Mostly I think I’d be interest in something that only checks dependencies, not dev-dependencies. Is that possible?

It is, it could look something like

{
  "extends": [
    "config:base",
    ":preserveSemverRanges", // keep ~ and ^ ranges
    "schedule:weekly" // only open PRs early monday morning
  ],
  "devDependencies": {
     "enabled": false // don't auto update dev dependencies
  }
}

There'd also be the option to group PRs from Unified repos.
E.G. if a repo depends on remark-parse, remark-stringify, and remark-frontmatter.
All three would be updated in a single PR.

{
  "extends": [
    "config:base",
    ":preserveSemverRanges", // keep ~ and ^ ranges
    "schedule:weekly" // only open PRs early monday morning
  ],
  "devDependencies": {
     "enabled": false // don't auto update dev dependencies
  },
  "packageRules": [ // group unified project dependencies together
    {
      "groupName": "unified",
      "packagePatterns": [
        "^unified"
      ]
    },
    {
      "groupName": "remark",
      "packagePatterns": [
        "^remark"
      ]
    },
    {
      "groupName": "rehype",
      "packagePatterns": [
        "^rehype"
      ]
    },
    {
      "groupName": "redot",
      "packagePatterns": [
        "^redot"
      ]
    }
  ]
}

there'd also be the option to create a renovate-config-unified or unified-config-wooorm to allow all projects to have their renovate settings managed at one time.
https://renovatebot.com/docs/config-presets/#preset-hosting

@ChristianMurphy
Copy link
Member

Dependabot allows for scheduling update PRs to be opened weekly.
It does not currently have options for skipping dev dependency updates, grouping PRs, or shared configuration.

@ChristianMurphy
Copy link
Member

@wooorm What would be the next step for this?
Is this something that could be setup directly, or is this something that would be a better fit for the new RFC process?

@wooorm
Copy link
Member Author

wooorm commented Jun 1, 2019

So here are my thoughts, what do you think?

I’d say try it out for a month and circle back how it went?! We could start out in just one org, syntax-tree, try a config in a repo there (could we use syntax-tree/.github?).
We could later either start pointing from each package to a unified preset, or point the per-org preset to a unified preset?
Seems like renovate can do what’s needed (I personally find the deps/dev-deps differentiation important). Not sure if the grouping is needed yet? We can try that out later when it works and we’re switching everything over?

@wooorm wooorm added ⛵️ status/released 🗄 area/interface This affects the public interface 🦋 type/enhancement This is great to have 🧑 semver/major This is a change labels Aug 12, 2019
@wooorm wooorm added the 💪 phase/solved Post is done label Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗄 area/interface This affects the public interface 💪 phase/solved Post is done 🧑 semver/major This is a change 🦋 type/enhancement This is great to have
Development

Successfully merging a pull request may close this issue.

2 participants