fix(base): introduce static version property for component class #3582

crav710 · 2023-08-24T06:25:02Z

Description

Adding Static property for component classes which can be useful for component libraries to handle scenarios like version conflict.

Related issue(s)

fixes [Feat]: Introduce publicly-accessible version to SWC components #3574
Discussion :: Static version property for component classes #3556

Motivation and context

Adding version to SWC components would help component library to make decision in below scenarios ::

If the registered component's version is semver compatible with the component libraries' dependency, there's no need to register the library's version of the component
If the registered component's version is not semver compatible the library might register and use the component using a custom tag name.
The component library could possibly smooth over backwards-incompatible changes by knowing what the registered version is and work around compatibility issues.

Versioning would also help in long term approach for custom scoped registries as discussed in #3556

How has this been tested?

Unit test for base.test has been written to check the version is present in SpectrumElement
Tested via storybook to check if the version is set correctly.

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
If my change required a change to the documentation, I have updated the documentation in this pull request.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.
I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Best practices

This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.

github-actions · 2023-08-24T06:28:49Z

Branch Preview

Visual regression test results

When a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:

github-actions · 2023-08-24T08:11:39Z

Tachometer results

Chrome

accordion permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	405 kB	253.36ms - 257.23ms	-	unsure 🔍 -1% - +1% -2.73ms - +2.06ms
branch	400 kB	254.22ms - 257.04ms	unsure 🔍 -1% - +1% -2.06ms - +2.73ms	-

action-bar permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	476 kB	120.31ms - 121.27ms	-	unsure 🔍 -1% - +0% -1.26ms - +0.44ms
branch	471 kB	120.50ms - 121.90ms	unsure 🔍 -0% - +1% -0.44ms - +1.26ms	-

action-button permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	506 kB	233.17ms - 235.85ms	-	slower ❌ 8% - 10% 17.37ms - 20.77ms
branch	502 kB	214.40ms - 216.49ms	faster ✔ 7% - 9% 17.37ms - 20.77ms	-

action-group permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	527 kB	132.54ms - 134.95ms	-	unsure 🔍 -2% - +1% -2.18ms - +1.76ms
branch	522 kB	132.39ms - 135.51ms	unsure 🔍 -1% - +2% -1.76ms - +2.18ms	-

action-menu permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	624 kB	245.69ms - 248.90ms	-	faster ✔ 1% - 3% 1.61ms - 6.33ms
branch	620 kB	249.53ms - 253.00ms	slower ❌ 1% - 3% 1.61ms - 6.33ms	-

Firefox

accordion permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	405 kB	543.67ms - 572.77ms	-	unsure 🔍 -3% - +5% -15.39ms - +27.47ms
branch	400 kB	536.45ms - 567.91ms	unsure 🔍 -5% - +3% -27.47ms - +15.39ms	-

action-bar permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	476 kB	342.66ms - 365.66ms	-	unsure 🔍 -5% - +4% -19.66ms - +14.22ms
branch	471 kB	344.45ms - 369.31ms	unsure 🔍 -4% - +6% -14.22ms - +19.66ms	-

action-button permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	506 kB	502.63ms - 534.85ms	-	unsure 🔍 -5% - +5% -24.89ms - +23.57ms
branch	502 kB	501.30ms - 537.50ms	unsure 🔍 -5% - +5% -23.57ms - +24.89ms	-

action-group permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	527 kB	334.98ms - 367.34ms	-	unsure 🔍 -4% - +9% -12.57ms - +29.81ms
branch	522 kB	328.85ms - 356.23ms	unsure 🔍 -8% - +3% -29.81ms - +12.57ms	-

action-menu permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	624 kB	497.15ms - 526.81ms	-	unsure 🔍 -5% - +4% -23.48ms - +19.84ms
branch	620 kB	498.01ms - 529.59ms	unsure 🔍 -4% - +5% -19.84ms - +23.48ms	-

.husky/pre-commit

Westbrook

Some nitty things, but really close. Seems like we'd be in a good place to test this out on the next release after those small points of clean up.

tools/base/src/Base.ts

Westbrook · 2023-09-06T14:18:18Z

tools/.eslintrc.json

+            "files": ["./base/src/version.js"],
+            "rules": {
+                "notice/notice": "off"
+            }


Why was this needed?

If the generator can't add this information, should we add this file to .eslintignore instead?

Or I guess the question is why isn't listing it there enough?

I think the intension here is to allow the linter to run on both the files (d.ts & .js) as tools/**/*.d.ts excludes tools/base/src/version.d.ts file from tools in .eslintignore
Then to ignore the header error for version.js we had to add the above override rule inside .eslintrc.json.

I believe we can safely remove the last line !tools/base/src/version.js as it's included by default. Although if we just add tools/base/src/version.js to .eslintignore we would not require the above change in .eslintrc.json.

@rickharris why is it necessary to add version.d.ts & version.js files to linter ?

Yeah I think that we can eslintignore the files if we'd like to. I also think you're right that we can remove !tools/base/src/version.js from the eslintignore file. Either option should be fine, just a preference of if we want to ignore the whole file or just the header rule.

@Westbrook added tools/base/src/version.js to .eslintignore & removed the header override.

Are we able to remove the updates to this file with that change?

Yes Removed it.

Westbrook · 2023-09-06T14:18:39Z

tools/base/test/base.test.ts

@@ -10,6 +10,7 @@ governing permissions and limitations under the License.
 */
 import { SpectrumElement } from '@spectrum-web-components/base';
 import { elementUpdated, expect, fixture, html } from '@open-wc/testing';
+import { version } from '../src/version';


File extension.

@Westbrook After adding the file extensions I see that all tests are failing reason being the version.js is being created/updated using genversion & it's excluded from build so version.dev.js is not created & all the test's fail.
Is it okay to roll back to removing extension from version as only version.js file is being created ?

File extensions are required. Checkout my other note re *.dev.js files.

tools/base/src/Base.ts

Westbrook

CI is failing because by default our tooling expects there to be both a version.js file AND a version.dev.js file. The laziest answer is to give it what it wants and build both files in the pre-commit. Alternatively, we could add specific support for version.js in this script so that the expectation of the version.dev.js file is not created. Both are a tiny bit awkward, so I'm fine with either.

crav710 · 2023-09-08T16:06:32Z

@Westbrook Added the support in script by removing the development condition similar to .css.

Westbrook

Let's try it. If it does work for the team, I'd love for us to go back and document it's usage, and how other teams might override it, etc. but for now we can keep it more on the quiet side.

…version.js

…o .eslintignore

* fix(base): add static version property * chore: version update by commit * refactor: added version.js to eslintignore and updated file extensions * fix(base): fixed unit test case & removed development condition from version.js * refactor: removed notice rule override as version.js has been added to .eslintignore --------- Co-authored-by: Rick Harris <rickharris@users.noreply.github.com> Co-authored-by: Ravi Sharma <ravi.sharma@adobe.com>

crav710 requested a review from Westbrook August 24, 2023 06:25

crav710 mentioned this pull request Aug 24, 2023

feat(base): introduce static version property for component class #3579

Closed

11 tasks

crav710 commented Aug 29, 2023

View reviewed changes

.husky/pre-commit Show resolved Hide resolved

crav710 requested a review from benjamind August 29, 2023 15:32

Westbrook reviewed Sep 6, 2023

View reviewed changes

crav710 requested a review from Westbrook September 8, 2023 09:18

Westbrook reviewed Sep 8, 2023

View reviewed changes

crav710 requested a review from Westbrook September 8, 2023 16:08

Westbrook approved these changes Sep 8, 2023

View reviewed changes

rickharris and others added 5 commits September 8, 2023 16:46

feat(base): add static version property

dd1fef6

chore: version update by commit

2512a52

refactor: added version.js to eslintignore and updated file extensions

52a2ff9

fix(base): fixed unit test case & removed development condition from …

432f106

…version.js

refactor: removed notice rule override as version.js has been added t…

0071ff7

…o .eslintignore

Westbrook force-pushed the crav710/3574-static-version-support branch from fb302ac to 0071ff7 Compare September 8, 2023 20:46

Westbrook changed the title ~~feat(base): Introduce static version property for component class~~ fix(base): introduce static version property for component class Sep 8, 2023

Westbrook merged commit e7e2b76 into main Sep 8, 2023
44 of 46 checks passed

Westbrook deleted the crav710/3574-static-version-support branch September 8, 2023 20:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(base): introduce static version property for component class #3582

fix(base): introduce static version property for component class #3582

crav710 commented Aug 24, 2023 •

edited by Westbrook

github-actions bot commented Aug 24, 2023

github-actions bot commented Aug 24, 2023 •

edited

accordion permalink

action-bar permalink

action-button permalink

action-group permalink

action-menu permalink

accordion permalink

action-bar permalink

action-button permalink

action-group permalink

action-menu permalink

Westbrook left a comment

Westbrook Sep 6, 2023

Westbrook Sep 6, 2023

Westbrook Sep 6, 2023

crav710 Sep 7, 2023

rickharris Sep 7, 2023

crav710 Sep 8, 2023

Westbrook Sep 8, 2023

crav710 Sep 8, 2023

Westbrook Sep 6, 2023

crav710 Sep 8, 2023

Westbrook Sep 8, 2023

Westbrook left a comment

crav710 commented Sep 8, 2023

Westbrook left a comment

fix(base): introduce static version property for component class #3582

fix(base): introduce static version property for component class #3582

Conversation

crav710 commented Aug 24, 2023 • edited by Westbrook

Description

Related issue(s)

Motivation and context

How has this been tested?

Screenshots (if appropriate)

Types of changes

Checklist

Best practices

github-actions bot commented Aug 24, 2023

Branch Preview

github-actions bot commented Aug 24, 2023 • edited

Tachometer results

accordion permalink

action-bar permalink

action-button permalink

action-group permalink

action-menu permalink

accordion permalink

action-bar permalink

action-button permalink

action-group permalink

action-menu permalink

Westbrook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Westbrook left a comment

Choose a reason for hiding this comment

crav710 commented Sep 8, 2023

Westbrook left a comment

Choose a reason for hiding this comment

crav710 commented Aug 24, 2023 •

edited by Westbrook

github-actions bot commented Aug 24, 2023 •

edited