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

Convert default themes to JSX; merge into typedoc repo #1634

Closed
wants to merge 33 commits into from

Conversation

cspotcode
Copy link
Contributor

@cspotcode cspotcode commented Jul 18, 2021

Replacing TypeStrong/typedoc-default-themes#137

EDIT next tasks to get this PR ready to merge:

  • decide where theme source assets should live, currently in /src/lib/output/themes/
  • decide when theme webpack builds should run
    • wire into npm run build?
    • relocate webpack.config.js files?
  • decide where built theme assets should live, currently in /dist/lib/output/themes/bin
  • remove <md> emit; emit a div instead
  • remove prettier, postprocessing find-and-replace, and emitting to expected/actual from renderer.test.ts
    • before we do this, I want to make sure stakeholders are happy with the current output
  • overwrite specs with the JSX output
    • will be ugly because JSX does not care about emitting newlines. Same blocker as above: wait for stakeholder approval
  • refactor __partials__ into an instance of DefaultThemePartials
    • add partials getter to the DefaultTheme; wrap all partials in factory that binds them to the theme / partials.
  • fix MarkedPlugin injection
    • is currently a global state hack
  • fix MarkedPlugin.sources and MarkedPlugin.outputFileName
    • are currently pulled from handlebars context and stored before markdown rendering or highlighting
    • they are only used when logging an error message: can we remove the info from the message?
  • demote MarkedPlugin and LayoutPlugin to not be plugins; merge with Theme
    • default theme can instantiate and keep a reference to MarkedHelpers
    • Top-level templates can do layout themselves; no need for a second pass to "wrap" the page
    • Wound up keeping MarkedPlugin as a plugin, grabbing it via getComponent
  • fix isExported to match review feedback?
  • remove hack() in Partials
    • update all partial factories to accept reference to partials separately from bindings

Future work

Not necessary in this PR, but it's nice to get excited about future possibilities:

  • deduplicate all the token <spans>: {, [, ], etc.
  • remove all redundant <></> fragment wrappers, {} wrappers, etc.

@cspotcode cspotcode marked this pull request as draft July 18, 2021 04:46
tsconfig.json Outdated Show resolved Hide resolved
@@ -216,6 +216,10 @@ export class ReflectionFlags extends Array<string> {
return this.hasFlag(ReflectionFlag.Readonly);
}

get isExported() {
return TODO as boolean;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was being called from templates but not declared anywhere. Do you know what the implementation should be?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This was removed in 0.20, it's always true now.

@cspotcode
Copy link
Contributor Author

It works! Tests have been (temporarily) updated to emit prettified versions of both the spec html and the typedoc output for visual inspection. These outputs match:

https://github.com/cspotcode/typedoc/compare/jsx-progress-report/expected..cspotcode:jsx-progress-report/actual

Next I'm going to render the spec output from master and from this pull request, and use a visual inspection tool to check for differences.

@cspotcode
Copy link
Contributor Author

cspotcode commented Jul 27, 2021

Notes about this PR:

it is currently taking the expected and the actual html and formatting both with prettier and some find-and-replace. We'll need to remove that and replace all the specs prior to merging this. Tests run way slower doing this postprocessing.

In a handful of places, markdown is wrapped in an <md> element. We'll need to change that to a <div>.

@cspotcode
Copy link
Contributor Author

Also need to figure out how and when the webpack configs run; wire them into the build.

@cspotcode
Copy link
Contributor Author

TODO use osnap instead of percy for visual testing

@cspotcode
Copy link
Contributor Author

cspotcode commented Jul 28, 2021

Added scripts for visual regression testing. Pushed the generated report here: https://github.com/cspotcode/typedoc/tree/jsx-visual-regression-report

  1. Capture screenshots of master build of ./dist/tmp/test
  • Not explained here, but basically modify and then run the ./src/test/capture-screenshots.ts. Modify it to capture the output of master ./dist/tmp/test, wherever that is on your filesystem
  1. Copy those screenshots to ./dist/tmp/.reg/expected
  • This might do the trick: npm run test:visual-regression-report:copy-actual-to-expected
  1. Now run tests of this branch and generate a comparison report
npm run build
PRESERVE_OUTPUT_FOR_VISUAL_REGRESSION_TEST=true npm test
npm run test:visual-regression-report

Open dist/tmp/.reg/index.html to view the report UI

@cspotcode
Copy link
Contributor Author

I updated the op with a checklist of tasks.

@cspotcode cspotcode marked this pull request as ready for review July 29, 2021 12:59
Copy link
Collaborator

@Gerrit0 Gerrit0 left a comment

Choose a reason for hiding this comment

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

decide where theme source assets should live, currently in /src/lib/output/themes/
decide where built theme assets should live, currently in /dist/lib/output/themes/bin

I'm fine with this, can always change later... not a public path.

they are only used when logging an error message: can we remove the info from the message?

I think it is still useful, will likely be moving eventually once I get around to creating a proper comment parser that's a bit smarter than regex.

fix isExported to match review feedback?

Likely just going to end up removing this

} else {
return contents;
}
// if (path.substr(-4).toLocaleLowerCase() === ".hbs") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dead code

@@ -1,7 +1,7 @@
{
"compilerOptions": {
"module": "CommonJS",
"lib": ["ES2018"],
"lib": ["ES2018", "dom"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hoping to avoid needing to do this... probably time typedoc started using project references. The themes have dom, but not typedoc itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it make sense to publish the themes as separate packages with a simple monorepo setup? It would make the themes more clearly look like how a third-party theme might be implemented.

@@ -8,8 +8,10 @@ export interface DemoProps {
export class Demo {
private foo: number;

//@ts-ignore
Copy link
Collaborator

Choose a reason for hiding this comment

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

Necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it was complaining that props is never used. I could add the same this.foo trick for this.props

@Gerrit0 Gerrit0 mentioned this pull request Jul 31, 2021
Copy link
Contributor Author

@cspotcode cspotcode left a comment

Choose a reason for hiding this comment

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

they are only used when logging an error message: can we remove the info from the message?

I think it is still useful, will likely be moving eventually once I get around to creating a proper comment parser that's a bit smarter than regex.

Ok, might need to refactor DefaultThemeRenderContext a bit. The current implementation uses a singleton RenderContext for each page, but we can instead create a new RenderContext for every page, so that the RenderContext has information about the page -- maybe a reference to the PageEvent? -- which it can expose to the partials.

If we're going to create a new RenderContext for each page, then we can reduce the work being done in the constructor. Instead of each partial being a factory wrapper around the partial -- (bindings: Bindings) => (reflection: Reflection) => <> -- we can simplify to a single function which accepts both bindings and reflection.

I had thought of inlining all partials as methods of the RenderContext, but I feel like splitting into separate files is easier to follow. I dunno, I'm not sure which is better.

@@ -8,8 +8,10 @@ export interface DemoProps {
export class Demo {
private foo: number;

//@ts-ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it was complaining that props is never used. I could add the same this.foo trick for this.props

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Sep 10, 2021

I'm rather confused as to why GitHub didn't pick this up as merged, it's been merged and now released as 0.22.0! Thanks again :)

@Gerrit0 Gerrit0 closed this Sep 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants