cli icon indicating copy to clipboard operation
cli copied to clipboard
verified publisher icon all-contributors

refactor: add tbody to contributors table

Open hpierre74 opened this issue 3 years ago • 4 comments

What: The contributors table generator

Why: Browsers are inserting tbody inside tables when it is not found, it is not really enforced by W3C to add tbody but since MDX will complain because of the react usage beneath it, I think this change has no negative impact and can fix an error in some docusaurus (or other MDX solutions). FYI: When react reconciles the tree it finds a difference in the markup since browsers add the tbody, leading to an error

How: Simply wrap the tr inside a tbody in the generator html string

Checklist:

  • [ ] Documentation
  • [ ] Tests
  • [ ] Ready to be merged
  • [ ] Added myself to contributors table

hpierre74 avatar Jan 07 '22 11:01 hpierre74

Where do you even get that error?

Also, it might be worth fixing your PR so the failing CI check passes.

Berkmann18 avatar Jan 07 '22 18:01 Berkmann18

Where do you even get that error?

Using MDX instead of markdown. I have a Docusaurus site where I copy pasted the contributors table leading to a react error. I easily fixed it by hand but I thought this would be a harmless contribution

Also, it might be worth fixing your PR so the failing CI check passes.

Precisely why I'm using WIP in the title... I'll finish this ASAP

hpierre74 avatar Jan 08 '22 12:01 hpierre74

I'm failing to understand whats wrong with the CI check though

error [email protected]: The engine "node" is incompatible with this module. Expected version "^12.17.0 || ^14.13 || >=16.0.0". Got "12.14.0"

While package.json states:

  "chalk": "^4.0.0",

And yarn why says:

=> Found "[email protected]"
info Has been hoisted to "chalk"
info Reasons this module exists
   - Specified in "dependencies"
   - Hoisted from "@jest#console#chalk"
   - Hoisted from "@jest#types#chalk"
   - Hoisted from "jest-message-util#chalk"
   - Hoisted from "jest-util#chalk"
   - Hoisted from "inquirer#chalk"
   - Hoisted from "kcd-scripts#chalk"
   - Hoisted from "json-fixer#chalk"
   - Hoisted from "cz-conventional-changelog#@commitlint#load#chalk"
   - Hoisted from "kcd-scripts#babel-jest#chalk"
   - Hoisted from "kcd-scripts#eslint#chalk"
   - Hoisted from "kcd-scripts#husky#chalk"
   - Hoisted from "kcd-scripts#jest-watch-typeahead#chalk"
   - Hoisted from "kcd-scripts#rollup-plugin-size-snapshot#chalk"
   - Hoisted from "kcd-scripts#lint-staged#chalk"
   - Hoisted from "semantic-release#marked-terminal#chalk"
   - Hoisted from "cz-conventional-changelog#@commitlint#load#@commitlint#types#chalk"
   - Hoisted from "kcd-scripts#jest#@jest#core#chalk"
   - Hoisted from "kcd-scripts#babel-jest#@jest#transform#chalk"
   - Hoisted from "kcd-scripts#jest#jest-cli#chalk"
   - Hoisted from "kcd-scripts#@types#jest#jest-diff#chalk"
   - Hoisted from "kcd-scripts#jest-watch-typeahead#jest-watcher#chalk"
   - Hoisted from "kcd-scripts#lint-staged#log-symbols#chalk"
   - Hoisted from "semantic-release#@semantic-release#npm#npm#chalk"
   - Hoisted from "kcd-scripts#jest#@jest#core#@jest#reporters#chalk"
   - Hoisted from "kcd-scripts#jest-watch-typeahead#jest-watcher#@jest#types#chalk"
   - Hoisted from "kcd-scripts#jest#@jest#core#jest-config#chalk"
   - Hoisted from "kcd-scripts#jest#@jest#core#jest-resolve#chalk"
   - Hoisted from "kcd-scripts#jest#@jest#core#jest-runner#chalk"
   - Hoisted from "kcd-scripts#jest#@jest#core#jest-runtime#chalk"
   - Hoisted from "kcd-scripts#jest#@jest#core#jest-snapshot#chalk"
   - Hoisted from "kcd-scripts#jest-watch-typeahead#jest-watcher#jest-util#chalk"
   - Hoisted from "kcd-scripts#jest#@jest#core#jest-validate#chalk"
   - Hoisted from "kcd-scripts#jest#@jest#core#jest-watcher#chalk"
   - Hoisted from "semantic-release#@semantic-release#npm#npm#npm-audit-report#chalk"
   - Hoisted from "kcd-scripts#eslint-config-kentcdodds#eslint-plugin-jest-dom#@testing-library#dom#chalk"
   - Hoisted from "semantic-release#@semantic-release#npm#npm#libnpmexec#chalk"
   - Hoisted from "kcd-scripts#jest-watch-typeahead#jest-watcher#@jest#test-result#@jest#console#chalk"
   - Hoisted from "kcd-scripts#jest#@jest#core#jest-config#jest-jasmine2#chalk"
   - Hoisted from "kcd-scripts#jest#@jest#core#jest-snapshot#jest-matcher-utils#chalk"
   - Hoisted from "kcd-scripts#jest#@jest#core#jest-config#jest-jasmine2#jest-each#chalk"
   - Hoisted from "kcd-scripts#jest-watch-typeahead#jest-watcher#@jest#test-result#@jest#console#jest-message-util#chalk"
info Disk size without dependencies: "56KB"
info Disk size with unique dependencies: "108KB"
info Disk size with transitive dependencies: "192KB"
info Number of shared dependencies: 5
=> Found "cz-conventional-changelog#[email protected]"
info This module exists because "cz-conventional-changelog" depends on it.
info Disk size without dependencies: "44KB"
info Disk size with unique dependencies: "112KB"
info Disk size with transitive dependencies: "196KB"
info Number of shared dependencies: 6
=> Found "@babel/highlight#[email protected]"
info This module exists because "@babel#code-frame#@babel#highlight" depends on it.
info Disk size without dependencies: "44KB"
info Disk size with unique dependencies: "112KB"
info Disk size with transitive dependencies: "196KB"
info Number of shared dependencies: 6
=> Found "signale#[email protected]"
info This module exists because "semantic-release#signale" depends on it.
info Disk size without dependencies: "44KB"
info Disk size with unique dependencies: "112KB"
info Disk size with transitive dependencies: "196KB"
info Number of shared dependencies: 6
=> Found "concurrently#[email protected]"
info This module exists because "kcd-scripts#concurrently" depends on it.
info Disk size without dependencies: "72KB"
info Disk size with unique dependencies: "140KB"
info Disk size with transitive dependencies: "224KB"
info Number of shared dependencies: 6
=> Found "commitizen#[email protected]"
info Reasons this module exists
   - "cz-conventional-changelog#commitizen#cz-conventional-changelog" depends on it
   - Hoisted from "cz-conventional-changelog#commitizen#cz-conventional-changelog#chalk"
   - Hoisted from "cz-conventional-changelog#commitizen#inquirer#chalk"
info Disk size without dependencies: "44KB"
info Disk size with unique dependencies: "112KB"
info Disk size with transitive dependencies: "196KB"
info Number of shared dependencies: 6

hpierre74 avatar Jan 08 '22 14:01 hpierre74

Hi, may I check if there are any updates on this PR?

I faced a similar issue lately, the use case is as such:

  • I will use the all-contributors cli to modify both my README.md and a separate about.md file, which I render using a combination of custom markdown parser (markdown-it) and Vue to generate the final html. Due to the missing <tbody>, it causes a hydration issue from Vue:

[Vue warn]: The client-side rendered virtual DOM tree is not matching server-rendered content. This is likely caused by incorrect HTML markup, for example nesting block-level elements inside <p>, or missing <tbody>. Bailing hydration and performing full client-side render.

tlylt avatar May 03 '22 08:05 tlylt

I'm failing to understand whats wrong with the CI check though

error [email protected]: The engine "node" is incompatible with this module. Expected version "^12.17.0 || ^14.13 || >=16.0.0". Got "12.14.0"

While package.json states:

  "chalk": "^4.0.0",

And yarn why says:

=> Found "[email protected]"
info Has been hoisted to "chalk"
info Reasons this module exists
   - Specified in "dependencies"
   - Hoisted from "@jest#console#chalk"
   - Hoisted from "@jest#types#chalk"
   - Hoisted from "jest-message-util#chalk"
   - Hoisted from "jest-util#chalk"
   - Hoisted from "inquirer#chalk"
   - Hoisted from "kcd-scripts#chalk"
   - Hoisted from "json-fixer#chalk"
   - Hoisted from "cz-conventional-changelog#@commitlint#load#chalk"
   - Hoisted from "kcd-scripts#babel-jest#chalk"
   - Hoisted from "kcd-scripts#eslint#chalk"
   - Hoisted from "kcd-scripts#husky#chalk"
   - Hoisted from "kcd-scripts#jest-watch-typeahead#chalk"
   - Hoisted from "kcd-scripts#rollup-plugin-size-snapshot#chalk"
   - Hoisted from "kcd-scripts#lint-staged#chalk"
   - Hoisted from "semantic-release#marked-terminal#chalk"
   - Hoisted from "cz-conventional-changelog#@commitlint#load#@commitlint#types#chalk"
   - Hoisted from "kcd-scripts#jest#@jest#core#chalk"
   - Hoisted from "kcd-scripts#babel-jest#@jest#transform#chalk"
   - Hoisted from "kcd-scripts#jest#jest-cli#chalk"
   - Hoisted from "kcd-scripts#@types#jest#jest-diff#chalk"
   - Hoisted from "kcd-scripts#jest-watch-typeahead#jest-watcher#chalk"
   - Hoisted from "kcd-scripts#lint-staged#log-symbols#chalk"
   - Hoisted from "semantic-release#@semantic-release#npm#npm#chalk"
   - Hoisted from "kcd-scripts#jest#@jest#core#@jest#reporters#chalk"
   - Hoisted from "kcd-scripts#jest-watch-typeahead#jest-watcher#@jest#types#chalk"
   - Hoisted from "kcd-scripts#jest#@jest#core#jest-config#chalk"
   - Hoisted from "kcd-scripts#jest#@jest#core#jest-resolve#chalk"
   - Hoisted from "kcd-scripts#jest#@jest#core#jest-runner#chalk"
   - Hoisted from "kcd-scripts#jest#@jest#core#jest-runtime#chalk"
   - Hoisted from "kcd-scripts#jest#@jest#core#jest-snapshot#chalk"
   - Hoisted from "kcd-scripts#jest-watch-typeahead#jest-watcher#jest-util#chalk"
   - Hoisted from "kcd-scripts#jest#@jest#core#jest-validate#chalk"
   - Hoisted from "kcd-scripts#jest#@jest#core#jest-watcher#chalk"
   - Hoisted from "semantic-release#@semantic-release#npm#npm#npm-audit-report#chalk"
   - Hoisted from "kcd-scripts#eslint-config-kentcdodds#eslint-plugin-jest-dom#@testing-library#dom#chalk"
   - Hoisted from "semantic-release#@semantic-release#npm#npm#libnpmexec#chalk"
   - Hoisted from "kcd-scripts#jest-watch-typeahead#jest-watcher#@jest#test-result#@jest#console#chalk"
   - Hoisted from "kcd-scripts#jest#@jest#core#jest-config#jest-jasmine2#chalk"
   - Hoisted from "kcd-scripts#jest#@jest#core#jest-snapshot#jest-matcher-utils#chalk"
   - Hoisted from "kcd-scripts#jest#@jest#core#jest-config#jest-jasmine2#jest-each#chalk"
   - Hoisted from "kcd-scripts#jest-watch-typeahead#jest-watcher#@jest#test-result#@jest#console#jest-message-util#chalk"
info Disk size without dependencies: "56KB"
info Disk size with unique dependencies: "108KB"
info Disk size with transitive dependencies: "192KB"
info Number of shared dependencies: 5
=> Found "cz-conventional-changelog#[email protected]"
info This module exists because "cz-conventional-changelog" depends on it.
info Disk size without dependencies: "44KB"
info Disk size with unique dependencies: "112KB"
info Disk size with transitive dependencies: "196KB"
info Number of shared dependencies: 6
=> Found "@babel/highlight#[email protected]"
info This module exists because "@babel#code-frame#@babel#highlight" depends on it.
info Disk size without dependencies: "44KB"
info Disk size with unique dependencies: "112KB"
info Disk size with transitive dependencies: "196KB"
info Number of shared dependencies: 6
=> Found "signale#[email protected]"
info This module exists because "semantic-release#signale" depends on it.
info Disk size without dependencies: "44KB"
info Disk size with unique dependencies: "112KB"
info Disk size with transitive dependencies: "196KB"
info Number of shared dependencies: 6
=> Found "concurrently#[email protected]"
info This module exists because "kcd-scripts#concurrently" depends on it.
info Disk size without dependencies: "72KB"
info Disk size with unique dependencies: "140KB"
info Disk size with transitive dependencies: "224KB"
info Number of shared dependencies: 6
=> Found "commitizen#[email protected]"
info Reasons this module exists
   - "cz-conventional-changelog#commitizen#cz-conventional-changelog" depends on it
   - Hoisted from "cz-conventional-changelog#commitizen#cz-conventional-changelog#chalk"
   - Hoisted from "cz-conventional-changelog#commitizen#inquirer#chalk"
info Disk size without dependencies: "44KB"
info Disk size with unique dependencies: "112KB"
info Disk size with transitive dependencies: "196KB"
info Number of shared dependencies: 6

It states that it needs chalk to be v12.17 or above (no idea why).

Berkmann18 avatar Aug 16 '22 10:08 Berkmann18

I'm failing to understand whats wrong with the CI check though

error [email protected]: The engine "node" is incompatible with this module. Expected version "^12.17.0 || ^14.13 || >=16.0.0". Got "12.14.0"

It states that it needs chalk to be v12.17 or above (no idea why).

I think the v12.17 is referring to the node version, it needs to be 12.17 and above for chalk v5

tlylt avatar Aug 16 '22 13:08 tlylt

I'm failing to understand whats wrong with the CI check though

error [email protected]: The engine "node" is incompatible with this module. Expected version "^12.17.0 || ^14.13 || >=16.0.0". Got "12.14.0"

It states that it needs chalk to be v12.17 or above (no idea why).

I think the v12.17 is referring to the node version, it needs to be 12.17 and above for chalk v5

That's true; I misread that.

Berkmann18 avatar Sep 06 '22 20:09 Berkmann18

Somehow related to https://github.com/all-contributors/cli/pull/163

tenshiAMD avatar Sep 06 '22 23:09 tenshiAMD

@tenshiAMD I'm trying to figure this out as we speak, adding spaces there and there 😄

hpierre74 avatar Sep 06 '22 23:09 hpierre74

Now there is an unrelated test failing, I suspect this is a side effect of upgrading circleci node version, I'll look into it

[test] FAIL src/util/__tests__/url.js (10.5 s)
[test]   ● Throw an error when parsed input isn't a URL
[test] 
[test]     expect(received).toThrowError(expected)
[test] 
[test]     Expected substring: "Invalid URL: http://some string"
[test]     Received message:   "Invalid URL"
[test] 
[test]           18 |   }
[test]           19 |
[test]         > 20 |   const url = new URL(new RegExp('^\\w+\\:\\/\\/').test(input) ? input : `http://${input}`)
[test]              |               ^
[test]           21 |
[test]           22 |   if (!isHttpProtocol(url.protocol)) {
[test]           23 |     throw new TypeError('Provided URL has an invalid protocol')
[test] 
[test]       at parseHttpUrl (src/util/url.js:20:15)
[test]       at Object.<anonymous> (node_modules/expect/build/toThrowMatchers.js:83:11)
[test]       at Object.toThrowError (node_modules/expect/build/index.js:338:21)
[test]       at Object.<anonymous> (src/util/__tests__/url.js:48:48)
[test]       at Object.<anonymous> (src/util/__tests__/url.js:48:48)

hpierre74 avatar Sep 06 '22 23:09 hpierre74

Now there is an unrelated test failing, I suspect this is a side effect of upgrading circleci node version, I'll look into it

[test] FAIL src/util/__tests__/url.js (10.5 s)
[test]   ● Throw an error when parsed input isn't a URL
[test] 
[test]     expect(received).toThrowError(expected)
[test] 
[test]     Expected substring: "Invalid URL: http://some string"
[test]     Received message:   "Invalid URL"
[test] 
[test]           18 |   }
[test]           19 |
[test]         > 20 |   const url = new URL(new RegExp('^\\w+\\:\\/\\/').test(input) ? input : `http://${input}`)
[test]              |               ^
[test]           21 |
[test]           22 |   if (!isHttpProtocol(url.protocol)) {
[test]           23 |     throw new TypeError('Provided URL has an invalid protocol')
[test] 
[test]       at parseHttpUrl (src/util/url.js:20:15)
[test]       at Object.<anonymous> (node_modules/expect/build/toThrowMatchers.js:83:11)
[test]       at Object.toThrowError (node_modules/expect/build/index.js:338:21)
[test]       at Object.<anonymous> (src/util/__tests__/url.js:48:48)
[test]       at Object.<anonymous> (src/util/__tests__/url.js:48:48)

FYI the failure is due to the change in behavior for new URL() from node 12 to node 16 and above. E.g Node 12 ("Invalid URL: Hello world") image

Node 17 ("Invalid URL") image

tlylt avatar Sep 07 '22 00:09 tlylt

:tada: This PR is included in version 6.20.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

all-contributors-release-bot avatar Sep 07 '22 08:09 all-contributors-release-bot

Hi @tenshiAMD, wonder if you know whether this change will propagate to the all-contributor bot immediately or will need to wait for the https://github.com/all-contributors/app side to update?

tlylt avatar Sep 07 '22 08:09 tlylt

Hi @tenshiAMD, wonder if you know whether this change will propagate to the all-contributor bot immediately or will need to wait for the https://github.com/all-contributors/app side to update?

@tlylt not totally sure about this but I guess there is a slight delay thus it is always best to wait for few minutes or so.

tenshiAMD avatar Sep 07 '22 09:09 tenshiAMD

Hi @tenshiAMD, wonder if you know whether this change will propagate to the all-contributor bot immediately or will need to wait for the all-contributors/app side to update?

@tlylt not totally sure about this but I guess there is a slight delay thus it is always best to wait for few minutes or so.

Thanks @tenshiAMD !

Just tried it after a few hours and it doesn't seem to be updated: image

Perhaps the app side needs to update the cli dependency to the latest for this to work... Thanks anyway

tlylt avatar Sep 07 '22 11:09 tlylt

Perhaps the app side needs to update the cli dependency to the latest for this to work... Thanks anyway

@tlylt referring you to @Berkmann18. He might help us regarding this issue.

tenshiAMD avatar Sep 07 '22 13:09 tenshiAMD

@tlylt I made a PR for this, we are just waiting for @Berkmann18 or @gr2m to merge it. Thanks! 🎉

tenshiAMD avatar Sep 07 '22 17:09 tenshiAMD

@tlylt can you check again now? Please let us know. Thanks! 🎉

tenshiAMD avatar Sep 07 '22 20:09 tenshiAMD

@tlylt can you check again now? Please let us know. Thanks! 🎉

Tested and works well! Thank you for the follow-up 💯

tlylt avatar Sep 08 '22 01:09 tlylt