[bug] (addon-cssresources) - STORY_RENDERED causes picked CSS to reset

[pgoforth]

Issue:
Everytime the STORY_RENDERED event is fired for any reason (for example when a knob is changed through 'addon-knobs', or you change to a different story) the selected CSS resources are reset back to their default selected value.

What I did

• Added tracking to the current story id so 'addon-cssresources' doesn't reset itself on STORY_RENDERED messages that do not change the story.
• Reconfigured the loaded resource picked attribute so it is not reset when new CSS resources are loaded that share the same id with an existing resource.
• Removed defaulting list to an empty array in the render() method due to redundancy (we test for existence before the render)
• Added tests for css-resource-panel.tsx - 100% test coverage

[gaetanmaisse]

Is there a specific reason to code these tests in JS and not TS one? If something is not working do not hesitate to tell me and I will take a look!
If not just rename your file to .ts(or even .tsx in your case) 😉

[pgoforth]

The specific reason is that these unit tests do not require Typescript for any reason. Can you give me a reason to write these tests in Typescript...or more specifically, how writing these unit tests in Typescript would improve coverage or offer something that Javascript does not.

I'll take it a step further if you'd like. I can eliminate the JSX portion of the test as well by using:

const shallowNode = (props = defaultProps) =>
  shallow(React.createElement(CssResourcePanel, props, null));
const mountNode = (props = defaultProps) =>
  mount(React.createElement(CssResourcePanel, props, null));

Renaming a file to .ts when there is no Typescript seems silly to me. It makes people believe a file contains things that it doesn't.

[gaetanmaisse]

By writing rename it to .ts I was meaning migrate it to TS. 🔧

I totally agree with you on writing these unit tests in Typescript would not improve coverage nor offer something that Javascript does not ✅ but TS types checking improve productivity and give better maintainability in time.

What are the benefits for me?

• You can type your defaultProps object with CssResourcePanelProps and so if someone add, remove or update a prop she will instantly see that something is wrong without trying to run tests

• You will have types checking for jest and enzyme functions and a better intellisense in your IDE

• Refactoring using IDE features (rename, move, extract and so on) will be much safer and faster

@kroeder @ndelangen what is your opinion about it?

--

Anyway, it's a great and clean PR 👏

[ndelangen]

Ow I kept tests in JS since i was having difficulty getting jest to work with typescript at some point, If you gents can get it to work 💯 got for it!

There is no reason to keep them .js

[pgoforth]

@gaetanmaisse @ndelangen
I would argue that you should always keep your unit tests in vanilla Javascript for the following (very valid) reason.

• You can type your defaultProps object with CssResourcePanelProps and so if someone add, remove or update a prop she will instantly see that something is wrong without trying to run tests

This is valid for the codebase (which is typed) but not for unit tests. When writing unit tests, you need to be able to have the ability to provide invalid values for anything you pass into your code. This is because you have no control over whether the end user implementing your code is using Typescript to begin with.

• You will have types checking for jest and enzyme functions and a better intellisense in your IDE

Jest and Enzyme intellisense is provided by your IDE, not Typescript...and by Jest and Enzyme having tTypescript definitions. Since unit tests are not compiled into the codebase, nobody is going to be importing the unit tests, and therefore would not benefit from intellisense on them.

• Refactoring using IDE features (rename, move, extract and so on) will be much safer and faster

I'm not understanding this and how it applies to a unit test.

All these seems like reasons to develop a codebase in Typescript, but I see using Typescript on your unit tests as a potential detriment because you would lose the ability to test for incorrect types being provided by an end user, which is essential in some cases.

[codecov]

Codecov Report

Merging #6050 into next will increase coverage by 0.48%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##             next    #6050      +/-   ##
==========================================
+ Coverage   35.78%   36.27%   +0.48%     
==========================================
  Files         648      648              
  Lines        9520     9528       +8     
  Branches     1380     1351      -29     
==========================================
+ Hits         3407     3456      +49     
+ Misses       5536     5495      -41     
  Partials      577      577

Impacted Files	Coverage Δ
addons/cssresources/src/css-resource-panel.tsx	100% <100%> (+100%)	⬆️
addons/cssresources/src/constants.ts	100% <0%> (+100%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 017b18e...846809e. Read the comment docs.