fix(docs): respect custom README content when writing to a custom path #5648
Conversation
|
| Path | Error Count |
|---|---|
| src/dev-server/index.ts | 37 |
| src/dev-server/server-process.ts | 32 |
| src/compiler/prerender/prerender-main.ts | 22 |
| src/runtime/client-hydrate.ts | 20 |
| src/testing/puppeteer/puppeteer-element.ts | 20 |
| src/screenshot/connector-base.ts | 19 |
| src/runtime/vdom/vdom-render.ts | 17 |
| src/dev-server/request-handler.ts | 15 |
| src/compiler/prerender/prerender-optimize.ts | 14 |
| src/compiler/sys/stencil-sys.ts | 14 |
| src/sys/node/node-sys.ts | 14 |
| src/compiler/prerender/prerender-queue.ts | 13 |
| src/compiler/sys/in-memory-fs.ts | 13 |
| src/runtime/connected-callback.ts | 13 |
| src/runtime/set-value.ts | 13 |
| src/compiler/output-targets/output-www.ts | 12 |
| src/compiler/transformers/test/parse-vdom.spec.ts | 12 |
| src/compiler/transformers/transform-utils.ts | 12 |
| src/compiler/transpile/transpile-module.ts | 12 |
| src/mock-doc/test/attribute.spec.ts | 12 |
Our most common errors
| Typescript Error Code | Count |
|---|---|
| TS2322 | 361 |
| TS2345 | 344 |
| TS18048 | 204 |
| TS18047 | 82 |
| TS2722 | 37 |
| TS2532 | 24 |
| TS2531 | 21 |
| TS2454 | 14 |
| TS2790 | 11 |
| TS2352 | 9 |
| TS2769 | 8 |
| TS2538 | 8 |
| TS2416 | 7 |
| TS2493 | 3 |
| TS18046 | 2 |
| TS2684 | 1 |
| TS2430 | 1 |
Unused exports report
There are 14 unused exports on this PR. That's the same number of errors on main, so at least we're not creating new ones!
Unused exports
| File | Line | Identifier |
|---|---|---|
| src/runtime/bootstrap-lazy.ts | 21 | setNonce |
| src/screenshot/screenshot-fs.ts | 18 | readScreenshotData |
| src/testing/testing-utils.ts | 198 | withSilentWarn |
| src/utils/index.ts | 145 | CUSTOM |
| src/utils/index.ts | 269 | normalize |
| src/utils/index.ts | 7 | escapeRegExpSpecialCharacters |
| src/compiler/app-core/app-data.ts | 25 | BUILD |
| src/compiler/app-core/app-data.ts | 115 | Env |
| src/compiler/app-core/app-data.ts | 117 | NAMESPACE |
| src/compiler/fs-watch/fs-watch-rebuild.ts | 123 | updateCacheFromRebuild |
| src/compiler/types/validate-primary-package-output-target.ts | 61 | satisfies |
| src/compiler/types/validate-primary-package-output-target.ts | 61 | Record |
| src/testing/puppeteer/puppeteer-declarations.ts | 485 | WaitForEventOptions |
| src/compiler/sys/fetch/write-fetch-success.ts | 7 | writeFetchSuccessSync |
PR built and packed!Download the tarball here: https://github.com/ionic-team/stencil/actions/runs/8706228827/artifacts/1418224122 If your browser saves files to |
alicewriteswrongs
force-pushed
the
ap/custom-readme-content
branch
2 times, most recently
from
8e99131
to
b569ea7
Compare
test/docs-json/custom-readme-output/components/my-component/readme.md
Outdated
Show resolved
Hide resolved
| ? // The user set a custom `.dir` property, which is where we're going | ||
| // to write the updated README. We need to read the non-automatically | ||
| // generated content from that file and preserve that. | ||
| await getUserReadmeContent(compilerCtx, readmeOutputPath) |
There was a problem hiding this comment.
Would determining the README path at
make it such that we don't need to read the README file from disk again?There was a problem hiding this comment.
I think we can't really decide a 'canonical' README path at that point, because we're not operating on docs-readme output targets at that point
Or in other words, if I set a custom readme output dir on the docs-readme OT I don't think that should affect how the docs-json OT is handled
if we want that behavior then the readme location configuration should not be output target specific, but something in e.g. the @Component decorator
There was a problem hiding this comment.
I think we can't really decide a 'canonical' README path at that point, because we're not operating on docs-readme output targets at that point
But hasn't a user specified the canonical README at that point (and we've defaulted to the directory of the component if they haven't)? If the README.md is the source of truth, doesn't that always affect the docs-json OT, even today?
Maybe the question here is, how many README files should exist, and which ones influence 'docs-json'?
The Stencil component starter gives us a README by default for my-component, but on main today, that doesn't get regenerated/updated if we specify dir on the 'docs-readme' OT. With that in mind, I'd expect the README in the custom dir to be the be the 'canonical' one (and where we pull README data from for 'docs-json').
For example:
npm init stencil@latest component readme-out-test && cd $_ && npm ito generate a component starter- Modifying
stencil.config.tsbefore running a build:
diff --git a/stencil.config.ts b/stencil.config.ts
index 59cd15a..8167fb3 100644
--- a/stencil.config.ts
+++ b/stencil.config.ts
@@ -14,6 +14,7 @@ export const config: Config = {
},
{
type: 'docs-readme',
+ dir: 'my-custom-dir'
},
{
type: 'www',- Deleting
src/components/my-component/readme.mdthat was a part of the starter - Running a build with
npm run build
Would all result in the following dir tree:
my-custom-dir
└── components
└── my-component
└── readme.md
src
├── components
│ └── my-component
│ ├── my-component.css
│ ├── my-component.e2e.ts
│ ├── my-component.spec.ts
│ └── my-component.tsx
├── components.d.ts
├── index.html
├── index.ts
└── utils
├── utils.spec.ts
└── utils.ts
Where README.md doesn't exist in src, because we don't generate it
Does that make sense? I've been staring at Jira for far too long today, and my brain feels a little static-y 😆
As an aside, the behavior today on main and this branch creates a bit of a conundrum - we've left a README file next to the component in the filesystem, with no real way to clean it up ourselves (users will just have to do that). We have the same behavior on this branch, but I think that's out of scope here.
There was a problem hiding this comment.
Part of the problem is that the way this is built we just don't have a 'canonical' README location or a way to specify that at present (or, we don't have one without making some assumptions that wouldn't be warranted in all cases). In particular, nothing prevents me from adding multiple different docs-readme output targets, each with different dir properties. Stencil will handle them all, and with this change for each one it will 'respect' any custom content that the user has in each readme - this is in line with generally how our output target processing is based on "give me all of the dist OTs and I'll process them," but it does mean that I don't think we can define a canonical readme location right now based on how this thing works without making some assumption about what the user 'means'.
Right now in the docs-json output target (i.e. the code in src/compiler/docs/generate-doc-data.ts there) we have an assumption that the readme next to the component is the 'canonical' one. We could add a check where we make some assumptions about the user wants, and do something like
- is there a single docs-readme OT on the config?
- does that have the
dirtarget?
if yes on both counts, then use the readme at that path as the readme for the docs-json OT (and just in general). We could do that, but we'd then need to look at maybe adding validation that you only specify one docs-readme OT, or designate a 'primary' one, or just use the first one silently, etc.
Thinking through some of the additional complexity here makes me hesitant to go forward with a change like this right now, I think especially it's out of scope for this PR which is just patching the existing functionality for the docs-readme OT itself. I will add a ticket to the backlog to consider whether we should approach this differently in the future / add a concept of a 'canonical' readme that's 1:1 associated with a file.
There was a problem hiding this comment.
In particular, nothing prevents me from adding multiple different docs-readme output targets, each with different dir properties.
Ahhh that's what I was missing - I was only thinking about there being 0-1, and multiple READMEs scattered around the project directory as a result of multiple runs of stencil build --docs (and the like) with difference configurations.
I agree with your assessment there - thanks for the explanation!
alicewriteswrongs
force-pushed
the
ap/custom-readme-content
branch
from
640fad4
to
fbe8148
Compare
There was a problem hiding this comment.
Looking at the reproduction case I was able to verify that given patch changes the behavior as following:
Before:
Content within the src/components/my-component/readme.md file was applied to the output file (e.g. output/components/my-component/readme.md) if a .dir property was set in the stencil.config.ts.
After:
None of what is being written in src/components/my-component/readme.md impacts the output file (e.g. output/components/my-component/readme.md) if a .dir property was set in the stencil.config.ts.
On the `docs-readme` output target it's possible to set a custom output location with the `.dir` property and the README files generation for components will then be output to relative paths (like `my-component/readme.md`) within that directory. This fixes a bug where that behavior didn't properly respect any manually-entered content in those readme files, so that if, for instance, you set the output to `custom-readme-output` and had `"My Custom Text"` at the top of `custom-readme-output/components/my-component/readme.md` then running a build would overwrite your custom text. This changes things so that we read the content of the custom readme and use that as the basis for the new text that we're going to write to disk. This has the effect of preserving the custom text that a user might have input. fixes #5400 STENCIL-1185
alicewriteswrongs
force-pushed
the
ap/custom-readme-content
branch
from
fbe8148
to
ee375cd
Compare
|
Released in Stencil ♨️ v4.17.0 |
On the
docs-readmeoutput target it's possible to set a custom output location with the.dirproperty and the README files generation for components will then be output to relative paths (likemy-component/readme.md) within that directory.This fixes a bug where that behavior didn't properly respect any manually-entered content in those readme files, so that if, for instance, you set the output to
custom-readme-outputand had"My Custom Text"at the top ofcustom-readme-output/components/my-component/readme.mdthen running a build would overwrite your custom text.This changes things so that we read the content of the custom readme and use that as the basis for the new text that we're going to write to disk. This has the effect of preserving the custom text that a user might have input.
fixes #5400
What is the current behavior?
See #5400 for a description of the issue
What is the new behavior?
The content of README files in a custom output location is now respected.
Documentation
Does this introduce a breaking change?
Testing
Try out the reproduction case in #5400, and make sure you can reproduce the issue. Then build and install this branch and make sure it fixes the issue!