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

fix: outputReferenceFallbacks for css/variables formatter #1113

Merged
merged 2 commits into from
Mar 11, 2024

Conversation

tkgroot
Copy link

@tkgroot tkgroot commented Mar 4, 2024

Issue #, if available:

If options were provided with outputReferences and outputReferenceFallbacks set to true did not add the fallbacks when running with the css/variables formatter.

Description of changes:

  • outputReferenceFallbacks options are passed through to up until the createPropertyFormatter function, which was previously not the case
  • add test to verify outputReferenceFallbacks are set
  • update test snapshot

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@tkgroot tkgroot requested a review from a team as a code owner March 4, 2024 14:38
@tkgroot
Copy link
Author

tkgroot commented Mar 4, 2024

on a side note: I created the PR #1112 for the current version on the main branch as well. When I changed and worked with the repository on the main branch I found that running and debugging the tests was much more developer friendly than it is in the v4 branch. Finding a failing test in v4 is really cumbersome compared to the v3 on the main branch.
If I wouldn't have known what to fix because of my PR to the current main branch, I probably would have given up looking for it. Just wanted to share my DX with you.

@jorenbroekema
Copy link
Collaborator

Can you elaborate which part of the testing DX you struggled with?
The browser test runner should be quite developer friendly to use.

For some context, just to explain what has changed and why since v3 branch with regards to testing:
In v4 we now support browser usage out of the box without build tooling required and it became important to have our tests running across multiple environments: Windows, Linux, Node, Browser (chromium only atm). I had to find a way to run the same test in all of these environments.

The reason I switched away from Jest is because it also has poor (relies on Node's --experimental-vm-modules flag) ESM support and more importantly, doesn't support running tests in real browsers.
The browser test-runner I use doesn't have Jest test syntax support, but does have mocha, so I had to switch even for the Node tests if I want to reuse the same tests in both Node and Browser envs.

Mocha is unfortunately not too great at the moment with regards to ESM support, it's quite tricky to get it to run properly in watch-mode with ESM mochajs/mocha#4374 , and unfortunately I couldn't get "--parallel" working properly.

Another option I'm interested in is perhaps using Vitest, perhaps that one supports ESM / parallel / watch mode properly while also allowing to use Chai/Mocha syntax.

Anyway, your PR looks good, thanks :)! It seems this referencesFallback option was never implemented properly because I recall fixing something related to it a few months back in the type interfaces when I added first-class Typescript support.

@jorenbroekema
Copy link
Collaborator

jorenbroekema commented Mar 11, 2024

Ah and one more thing, can you run npx changeset (and then git add & push the changeset file) locally on your branch to add a human readable changelog entry for this change? A patch change is fine for this one

Edit: never mind I added one, I want to do a release in a couple of minutes so I figured I want to include this fix in it as well ;)

- outputReferenceFallbacks options are passed through to the
createPropertyFormatter function, which was previously not the case
- add test for fallback
- update snapshot

Signed-off-by: Tobias Kuppens Groot <tkuppensgroo@uos.de>
@jorenbroekema jorenbroekema force-pushed the fix/v4-outputReferenceFallbacks branch from 5263264 to 336c706 Compare March 11, 2024 12:09
@jorenbroekema jorenbroekema merged commit 72f020d into amzn:v4 Mar 11, 2024
4 checks passed
@tkgroot
Copy link
Author

tkgroot commented Mar 13, 2024

Sure, I'm happy to elaborate on this a bit more.
The most annoying thing I came across is that I wasn't able to hook my debugger to the test scripts. maybe a setup issue on my side, but on v3 it was working like a charm.
The second thing I found was less DXish was the logging, When I ran my change a test failed, It told me that it did, but not which one and because of the huge amount of logs created by the browser tests I had to really dig into them. Jest on v3 was a bit nicer actually pointing out the failing tests right away.
That's actually it, not much, but a small dent in my DX which I found annoying.

@jorenbroekema
Copy link
Collaborator

jorenbroekema commented Mar 14, 2024

Which test script where you running, the browser tests or the node tests? e.g. npm run test:node or npm run test:browser?

When debugging my preferred way is to run: npm run test:browser:watch and follow the CLI, e.g. if a failing test occurs you can press F and immediately into the failing test file to isolate the logs for only that file, or only a single test when using it.only(), and the runner also points out the file of the failing test
image

D and M are also super useful if you want to debug it in a browser e.g. when you're using debugger statements

For logging, I'm actually discussing some changes for it here #1039 in case you'd like to chip in with your feedback

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

2 participants