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

refactor: add tbody to contributors table #307

Merged
merged 4 commits into from
Sep 7, 2022

Conversation

hpierre74
Copy link
Contributor

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 hpierre74 changed the title refactor: add tbody to contributors table refactor: add tbody to contributors table (WIP) Jan 7, 2022
@Berkmann18
Copy link
Member

Where do you even get that error?

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

@hpierre74
Copy link
Contributor Author

hpierre74 commented Jan 8, 2022

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 hpierre74 changed the title refactor: add tbody to contributors table (WIP) refactor: add tbody to contributors table Jan 8, 2022
@hpierre74
Copy link
Contributor Author

hpierre74 commented Jan 8, 2022

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

error chalk@5.0.0: 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 "chalk@4.1.2"
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#chalk@2.4.2"
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#chalk@2.4.2"
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#chalk@2.4.2"
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#chalk@2.4.2"
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#chalk@2.4.2"
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

@tlylt
Copy link

tlylt commented May 3, 2022

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.

@Berkmann18
Copy link
Member

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

error chalk@5.0.0: 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 "chalk@4.1.2"
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#chalk@2.4.2"
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#chalk@2.4.2"
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#chalk@2.4.2"
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#chalk@2.4.2"
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#chalk@2.4.2"
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).

@tlylt
Copy link

tlylt commented Aug 16, 2022

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

error chalk@5.0.0: 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

@Berkmann18
Copy link
Member

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

error chalk@5.0.0: 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.

@hpierre74 hpierre74 force-pushed the tbody-addition branch 2 times, most recently from 6328d33 to 19bb6b0 Compare September 6, 2022 22:27
Copy link
Member

@tenshiAMD tenshiAMD left a comment

Choose a reason for hiding this comment

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

@hpierre74 Hi, awesome man! validate_and_deploy is now working again. Please check the requested changes. Thanks! 🎉

newContent => {
return `\n<table>\n <tr>\n ${newContent}\n </tr>\n</table>\n\n`
return `\n<table>\n \n<tbody>\n <tr>\n ${newContent}\n </tr>\n </tobdy>\n </table>\n\n`
Copy link
Member

Choose a reason for hiding this comment

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

@hpierre74 looks like need to write a test for this one

@@ -55,9 +55,9 @@ function generateContributorsList(options, contributors) {
}),
_.chunk(options.contributorsPerLine),
_.map(formatLine),
_.join('\n </tr>\n <tr>\n '),
_.join('\n </tr>\n <tr>\n '),
Copy link
Member

Choose a reason for hiding this comment

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

@hpierre74 also need test for this one

@tenshiAMD
Copy link
Member

Somehow related to #163

@hpierre74
Copy link
Contributor Author

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

@hpierre74
Copy link
Contributor Author

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)

Copy link
Member

@tenshiAMD tenshiAMD left a comment

Choose a reason for hiding this comment

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

@hpierre74 LGTM however I noticed some changes. Why are some unnecessary lines got changed? Are you using a custom .editorconfig plugin or something?

Copy link
Member

@tenshiAMD tenshiAMD left a comment

Choose a reason for hiding this comment

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

@hpierre74 LGTM however I noticed some changes. Why are some unnecessary lines got changed? Are you using a custom .editorconfig plugin or something? I think we can remove those first then we can merge this one. Cheers! 🎉

src/util/__tests__/url.js Show resolved Hide resolved
src/util/__tests__/url.js Show resolved Hide resolved
src/util/__tests__/url.js Show resolved Hide resolved
src/generate/index.js Show resolved Hide resolved
src/util/__tests__/url.js Show resolved Hide resolved
src/util/__tests__/url.js Show resolved Hide resolved
@tlylt
Copy link

tlylt commented Sep 7, 2022

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

@tenshiAMD tenshiAMD merged commit deb6be9 into all-contributors:master Sep 7, 2022
@all-contributors-release-bot
Copy link
Member

🎉 This PR is included in version 6.20.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@tlylt
Copy link

tlylt commented Sep 7, 2022

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?

@tenshiAMD
Copy link
Member

tenshiAMD commented Sep 7, 2022

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.

@tlylt
Copy link

tlylt commented Sep 7, 2022

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

@tenshiAMD
Copy link
Member

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
Copy link
Member

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

@tenshiAMD
Copy link
Member

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

@tlylt
Copy link

tlylt commented Sep 8, 2022

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

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

@mtbc mtbc mentioned this pull request Sep 8, 2022
4 tasks
tenshiAMD added a commit that referenced this pull request Sep 13, 2022
* origin/master: (85 commits)
  refactor: log full error stack on error (#316)
  chore: fix status badges (#315)
  docs: add JoshuaKGoldberg as a contributor for bug (#314)
  fix: incorrect usage of `tbody` (#311)
  fix: trim `nextLink` before slicing (#309)
  fix: set default value as `7` for `contributorsPerLine` (#139)
  chore(deps): bump dependencies and devDeps (#298)
  refactor: add tbody to contributors table (#307)
  docs: add Lucas-C as a contributor for doc (#306)
  fix: scriptName + improving usage messages (#305)
  docs: add vapurrmaid as a contributor (#304)
  chore(deps): CVE-2021-23337 in inquirer->lodash (#303)
  docs: add SirWindfield as a contributor (#297)
  feat: add namespaced token (#296)
  docs: add LaChapeliere as a contributor (#292)
  feat(contribution-types): add research contribution type (#291)
  docs: add darekkay as a contributor (#290)
  feat: display a meaningful error when the config file is missing (#288)
  docs: add melink14 as a contributor (#285)
  docs: add jdalrymple as a contributor (#264)
  ...
qyurila added a commit to qyurila/prettier-eslint that referenced this pull request Oct 9, 2022
qyurila added a commit to qyurila/prettier-eslint that referenced this pull request Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants