-
Notifications
You must be signed in to change notification settings - Fork 795
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
fix(docs): respect custom README content when writing to a custom path #5648
Merged
+76
−8
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
22 changes: 22 additions & 0 deletions
22
test/docs-readme/custom-readme-output/components/styleurls-component/readme.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
# styleurls-component | ||
|
||
This file is in a custom location, set with `.dir` on the `docs-readme` OT. | ||
|
||
The content here above the 'auto-generation' comment shouldn't be overwritten. | ||
|
||
This is a regression test for the issue reported in ionic-team/stencil#5400. | ||
|
||
<!-- Auto Generated Below --> | ||
|
||
|
||
## CSS Custom Properties | ||
|
||
| Name | Description | | ||
| ------- | ------------ | | ||
| `--one` | Property One | | ||
| `--two` | Property Two | | ||
|
||
|
||
---------------------------------------------- | ||
|
||
*Built with [StencilJS](https://stenciljs.com/)* |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,5 +9,9 @@ export const config: Config = { | |
{ | ||
type: 'dist', | ||
}, | ||
{ | ||
type: 'docs-readme', | ||
dir: 'custom-readme-output', | ||
}, | ||
], | ||
}; |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would determining the README path at
stencil/src/compiler/docs/generate-doc-data.ts
Line 81 in b569ea7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 pointOr in other words, if I set a custom readme output dir on the
docs-readme
OT I don't think that should affect how thedocs-json
OT is handledif we want that behavior then the readme location configuration should not be output target specific, but something in e.g. the
@Component
decoratorThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 formy-component
, but onmain
today, that doesn't get regenerated/updated if we specifydir
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 i
to generate a component starterstencil.config.ts
before running a build:src/components/my-component/readme.md
that was a part of the starternpm run build
Would all result in the following dir tree:
Where
README.md
doesn't exist insrc
, because we don't generate itDoes 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 differentdir
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 insrc/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 likedir
target?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 onedocs-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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!