refactor: remove sub-eslintrc, fix more lint errors #7530
Conversation
Josh-Cena
added
the
pr: internal
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site settings. |
⚡️ Lighthouse report for the deploy preview of this PR
|
|
Size Change: -1 B (0%) Total Size: 797 kB ℹ️ View Unchanged
|
| // A very implicit API: we allow users to hand-write the toc variable. It will | ||
| // get overwritten in most cases, but until we find a better way, better keep | ||
| // supporting this | ||
| it('does not overwrite TOC var if no TOC', async () => { | ||
| const result = await processFixture('no-heading-with-toc-export'); | ||
| expect(result).toMatchSnapshot(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
@Josh-Cena I don't understand why this behavior was implemented.
As far as I understand:
Manually exported toc will be overriden by remark toc if there are headings:
## heading
export const toc = [{}]Manually exported toc won't be overriden by remark toc if there's no heading
export const toc = [{}]Why would we want 2 different behaviors here?
I'd like to get rid of one and always let the user override the toc with an explicit export.
Asking for #9684
There was a problem hiding this comment.
@slorber: It was implemented to support hand-writing TOC in very edge cases, such as importing partials, using exclusively JSX headings, and such. I think I implemented it this way, because if a user both has hand-written and existing TOC, it is more likely that they should be merged, but I'm okay with always-overriding behavior too. I just thought this is the least surprising for the time being.
There was a problem hiding this comment.
Thanks @Josh-Cena
After my refactor the logic is much easier to understand, and now feels a bit awkward to me.
I'll probably update this behavior and a few other things in another PR, for now I kept the existing behavior although 2 things surprised me:
- exporting the toc after the last import (instead of always at the end)
- overriding user explicit toc only if any heading is found
There was a problem hiding this comment.
exporting the toc after the last import (instead of always at the end)
This has always been the case, which surprised me too because it's extra work.
overriding user explicit toc only if any heading is found
Which to me is a slight improvement over always overriding explicit TOC, although I agree always respecting explicit TOC is probably the most reasonable.
There was a problem hiding this comment.
Thanks, I'll do those 2 changes in upcoming PRs.
I guess the toc override can be considered as a breaking change and should better be planned for v4? Or do you think it's fine for v3? I'm not sure since we never really documented this behavior anywhere so it might as well be a bugfix to make the behavior consistent.
There was a problem hiding this comment.
I think it has always remained an implementation detail, and while we did recommend using export toc = [...toc1, ...toc2] for the "barrel of partials" case before, this use case will keep working. I don't think anyone will expect their explicit TOC to be overridden.
Pre-flight checklist
Motivation
Little refactors, fix more errors.
I also included the type-aware configs as commented-out lines, so it can be more easily enabled in the future.
Test Plan
Test links
Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/
Related issues/PRs