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

feat(docs): add style mode to docs-json output #5718

Merged
merged 1 commit into from
May 3, 2024
Merged

Conversation

rwaskiewicz
Copy link
Member

@rwaskiewicz rwaskiewicz commented Apr 29, 2024

What is the current behavior?

GitHub Issue Number: N/A

What is the new behavior?

add a style mode to the output of the docs-json output target. when a stencil component is declared using the styleUrls property, a user may choose to apply styles depending on a "mode" that they set at runtime:

// https://stenciljs.com/docs/component#styleurls
import { Component } from '@stencil/core';

@Component({
  tag: 'todo-list',
  styleUrls: {
     ios: 'todo-list.ios.scss',
     md: 'todo-list.md.scss',
  }
})
export class TodoList {}

where the todo-list.ios.scss stylesheet will be applied with the mode is set to 'ios' at runtime, and todo-list.md.scss will be applied if the mode 'md' is set at runtime.

with this change, documented css properties will be associated with their respective modes in the output of the docs-json output target.

for todo-list.md.scss:

/**
 * @prop --button-background: Background of the button
 */
:host {}

the mode will now be reported in docs-json:

{
    "name": "--button-background",
    "annotation": "prop",
    "docs": "Background of the button",
+   "mode": "md"
},

if a property of the same name exists in more than one mode - e.g. if --button-background also existed in the ios mode stylesheet, two separate entries will be generated. this is accomplished by using a composite key for deduplicating/merging arrays consisting of the name and mode of the property/stylesheet

Documentation

https://github.com/ionic-team/stencil-site/pull/1417/files

Does this introduce a breaking change?

  • Yes
  • No

Testing

  1. Additional unit tests have been added
  2. The test/docs-json was rerun to rebuild its output file
  3. I tested this in Ionic Framework (expand section below for details)
Details (Click Me)

Changes to Framework

  1. Checkout 0873dc2ffbc623439a74926937a75f54a28b9709 in Framework, cd into core
  2. npm run build to get a baseline
  3. npm run build this branch, npm pack to tarball, npm i <TARBALL> in core
  4. Update stencil.config.ts
    diff --git a/core/stencil.config.ts b/core/stencil.config.ts
    index cdc5e76fe4..0415de6afa 100644
    --- a/core/stencil.config.ts
    +++ b/core/stencil.config.ts
    @@ -231,7 +231,7 @@ export const config: Config = {
         },
         {
           type: 'docs-json',
    -      file: '../packages/docs/core.json'
    +      file: '../packages/docs/core-new.json'
         },
         {
           type: 'dist-hydrate-script'
  5. npm run build to get a new docs-json file
  6. Diff the old docs-json and new docs-json file (attached)

core-json-output.txt

Other information

STENCIL-1269 CSS Documentation Should Account for Modes

Copy link
Contributor

github-actions bot commented Apr 29, 2024

--strictNullChecks error report

Typechecking with --strictNullChecks resulted in 1138 errors on this branch.

That's the same number of errors on main, so at least we're not creating new ones!

reports and statistics

Our most error-prone files
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 205
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 15 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 245 NODE_TYPES
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

Copy link
Contributor

github-actions bot commented Apr 29, 2024

PR built and packed!

Download the tarball here: https://github.com/ionic-team/stencil/actions/runs/8942412267/artifacts/1471362662

If your browser saves files to ~/Downloads you can install it like so:

unzip -d ~/Downloads ~/Downloads/stencil-core-4.17.2-dev.1714756050.df1fa90.tgz.zip && npm install ~/Downloads/stencil-core-4.17.2-dev.1714756050.df1fa90.tgz

@@ -283,12 +290,13 @@ auto-generated content

describe('default values', () => {
it.each(['', null, undefined])(
'defaults the annotation to an empty string if %s is provided',
"defaults the annotation to an empty string if '%s' is provided",
Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed this output was previously reading as:

defaults the annotation to an empty string if is provided

for the case of the empty string in one of my new tests, and went back and fixed it in a few places here. I can pull it out if folks wish, but it was only two or so tests affected - didn't seem worth it. LMK if you think otherwise, happy to pull it out

Copy link
Member

Choose a reason for hiding this comment

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

it's all good 👍

@@ -1,4 +1,13 @@
import { flatOne, isOutputTargetDocsJson, join, normalizePath, relative, sortBy, unique } from '@utils';
import {
DEFAULT_STYLE_MODE,
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only new import, just adding it gets us past 120 chars

@@ -164,7 +164,7 @@ export const extTransformsPlugin = (
// Set style docs
if (cmp) {
cmp.styleDocs ||= [];
mergeIntoWith(cmp.styleDocs, cssTransformResults.styleDocs, (docs) => docs.name);
mergeIntoWith(cmp.styleDocs, cssTransformResults.styleDocs, (docs) => `${docs.name},${docs.mode}`);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is probably OK as far as using a comma as the joiner here. I don't think any css property nor mode would ever have a comma in it, LMK if you think this isn't safe. Regardless, it would be easy to change in the future, as it's not exposed to users in our output

Copy link
Member

Choose a reason for hiding this comment

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

yeah I think it should be fine because I think even if there were a comma somewhere then the string produced should still be unique (that little function is only used for identifying when items are the same item)

rwaskiewicz added a commit to ionic-team/stencil-site that referenced this pull request Apr 29, 2024
add an example of the output for modes associated with a stylesheet.

associated with ionic-team/stencil#5718

STENCIL-1269
@rwaskiewicz rwaskiewicz marked this pull request as ready for review April 30, 2024 15:55
@rwaskiewicz rwaskiewicz requested a review from a team as a code owner April 30, 2024 15:55
Copy link
Member

@alicewriteswrongs alicewriteswrongs left a comment

Choose a reason for hiding this comment

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

I think this all looks good!

One question - would it make sense for us to add a mode to the component in test/docs-json so we can get an 'end-to-end' test of that showing up in the json output file?

EDIT: hit cmd-enter too early 😅

@@ -164,7 +164,7 @@ export const extTransformsPlugin = (
// Set style docs
if (cmp) {
cmp.styleDocs ||= [];
mergeIntoWith(cmp.styleDocs, cssTransformResults.styleDocs, (docs) => docs.name);
mergeIntoWith(cmp.styleDocs, cssTransformResults.styleDocs, (docs) => `${docs.name},${docs.mode}`);
Copy link
Member

Choose a reason for hiding this comment

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

yeah I think it should be fine because I think even if there were a comma somewhere then the string produced should still be unique (that little function is only used for identifying when items are the same item)

@@ -283,12 +290,13 @@ auto-generated content

describe('default values', () => {
it.each(['', null, undefined])(
'defaults the annotation to an empty string if %s is provided',
"defaults the annotation to an empty string if '%s' is provided",
Copy link
Member

Choose a reason for hiding this comment

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

it's all good 👍

@rwaskiewicz
Copy link
Member Author

@alicewriteswrongs Good point! Added in dd72a00

@rwaskiewicz
Copy link
Member Author

Hmmm....the output may not be deterministic in terms of ordering. I'll TAL at that later this afternoon (about to start a mini-meeting marathon)

@alicewriteswrongs
Copy link
Member

cool, yeah we may just need to add a sort somewhere

@rwaskiewicz
Copy link
Member Author

@alicewriteswrongs fixed it in a79fb63

Copy link
Member

@tanner-reits tanner-reits left a comment

Choose a reason for hiding this comment

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

LGTM!

rwaskiewicz added a commit to ionic-team/stencil-site that referenced this pull request May 3, 2024
add an example of the output for modes associated with a stylesheet.

associated with ionic-team/stencil#5718

STENCIL-1269
add a style mode to the output of the `docs-json` output target. when a
stencil component is declared using the `styleUrls` property, a user may
choose to apply styles depending on a "mode" that they set at runtime:
```tsx
// https://stenciljs.com/docs/component#styleurls
import { Component } from '@stencil/core';

@component({
  tag: 'todo-list',
  styleUrls: {
     ios: 'todo-list.ios.scss',
     md: 'todo-list.md.scss',
  }
})
export class TodoList {}
```

where the `todo-list.ios.scss` stylesheet will be applied with the mode
is set to 'ios' at runtime, and `todo-list.md.scss` will be applied if
the mode 'md' is set at runtime.

with this change, documented css properties will be associated with
their respective modes in the output of the `docs-json` output target.

for `todo-list.md.scss`:
```sass
/**
 * @prop --button-background: Background of the button
 */
:host {}
```

the mode will now be reported in `docs-json`:
```diff
{
    "name": "--button-background",
    "annotation": "prop",
    "docs": "Background of the button",
+   "mode": "md"
},
```

if a property of the same name exists in more than one mode - e.g. if
`--button-background` _also_ existed in the ios mode stylesheet, two
separate entries will be generated. this is accomplished by using a
composite key for deduplicating/merging arrays consisting of the name
and mode of the property/stylesheet

STENCIL-1269 CSS Documentation Should Account for Modes
@rwaskiewicz rwaskiewicz enabled auto-merge May 3, 2024 17:06
rwaskiewicz added a commit to ionic-team/stencil-site that referenced this pull request May 3, 2024
add an example of the output for modes associated with a stylesheet.

associated with ionic-team/stencil#5718

STENCIL-1269

---------

Co-authored-by: Christian Bromann <git@bromann.dev>
@rwaskiewicz rwaskiewicz added this pull request to the merge queue May 3, 2024
Merged via the queue into main with commit 44fcba1 May 3, 2024
129 checks passed
@rwaskiewicz rwaskiewicz deleted the rw/css-doc-mode branch May 3, 2024 17:22
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.

None yet

3 participants