Skip to content

Conversation

jttommy
Copy link
Contributor

@jttommy jttommy commented Jul 31, 2025

Allows for consts to be used within Listen and Event decorators through additional type checks in compiler

fixes: #6360

What is the current behavior?

GitHub Issue Number: (#6360)

What is the new behavior?

  • Allow @event and @listen decorators to use const variables and nested object constants
  • Add comprehensive test coverage with 15 test cases
  • Maintain backward compatibility for all existing decorators
  • Update type definitions with documentation and examples

Documentation

Does this introduce a breaking change?

  • Yes
  • No

Testing

  • Unit tests
  • e2e tests
  • Updated some other tests to exclude snapshot testing, which could be faulty. Instead we query DOM objects for precision.

Other information

Allows for consts to be used within Listen and Event decorators through additional type checks in compiler

fixes: stenciljs#6360
@jttommy jttommy requested a review from a team as a code owner July 31, 2025 16:42
@jttommy jttommy changed the title feat(compiler) feat(compiler): Allow consts for decorator arguments Jul 31, 2025
@johnjenkins
Copy link
Contributor

johnjenkins commented Jul 31, 2025

thanks for contributing @jttommy - looks great!

Do you mind:

  1. fixing the formatting issues
  2. adding some wdio / component tests for this (inside test/wdio) - specifically a test for shared listener / event variables imported into components would be valuable I think

TYSM

Adds wdio test for event work.
Runs linter and formatter
Adds propery comments to address lint issues
Addresses broken wdio config
Removes quotes for test script to allow running in multiple envs

fixes: stenciljs#6360
@jttommy
Copy link
Contributor Author

jttommy commented Aug 1, 2025

thanks for contributing @jttommy - looks great!

Do you mind:

  1. fixing the formatting issues
  2. adding some wdio / component tests for this (inside test/wdio) - specifically a test for shared listener / event variables imported into components would be valuable I think

TYSM

I believe I addressed the issues. WebDriverIO was giving me a lot of problems. I think those tests and that config need some work in another ticket.

increased the threshold for the scoped-css test to allow for testing environment latency
@jttommy
Copy link
Contributor Author

jttommy commented Aug 1, 2025

@johnjenkins
There are several failed WebDriver.io tests on main that predate my work. Particularly this:

[chrome 138.0.7204.158 windows #0-0] 1) dom-reattach should have proper values
[chrome 138.0.7204.158 windows #0-0] expect(received).toEqual(expected) // deep equality

- Expected  - 2
+ Received  + 2

- ObjectContaining {
+ Object {
    "parsed": Object {},
    "property": "border",
-   "value": "5px dotted rgb(255, 0, 0)",
+   "value": "4.8px dotted rgb(255, 0, 0)",
  }

It seems the rendered border-width is fairly sensitive and only off by a fraction of a pixel. I think maybe the hardware I'm running on is an issue? Or perhaps there's some other underlying CSS calculation that is off.

Furthermore there is a failed test regarding relative speed, which just assumes styles load quickly. I think this is a smelly test. I've upped the threshold on that timer.

@johnjenkins
Copy link
Contributor

johnjenkins commented Aug 1, 2025

hey thanks for looking at those @jttommy - I would say, as smelly as that test might seem, it's giving off some smoke with this MR.
I've just had a quick look on main and the allowance on it is very generous - it generally clears at around 10ms so for it to now be semi-consistently over 200ms leads me to think there's an issue here.
Could you have a look please?

The wdio test you mentioned doesn't seem to ever be an issue in CI - so, yes probably a local thing.

Limits decorator modifications to necessary decorators Listen and Event
@jttommy
Copy link
Contributor Author

jttommy commented Aug 4, 2025

hey thanks for looking at those @jttommy - I would say, as smelly as that test might seem, it's giving off some smoke with this MR. I've just had a quick look on main and the allowance on it is very generous - it generally clears at around 10ms so for it to now be semi-consistently over 200ms leads me to think there's an issue here. Could you have a look please?

The wdio test you mentioned doesn't seem to ever be an issue in CI - so, yes probably a local thing.

I updated the code to only apply to @Listen and @Event decorators to limit the scope of the work to what's relevant.

@jttommy jttommy requested a review from johnjenkins August 4, 2025 20:10
johnjenkins and others added 3 commits August 4, 2025 23:27
…and testability. Fixes dynamic import workaround for circular dependency in decorator-utils. Adds comprehensive comments to wdio dynamic import tests for clarity.
@jttommy
Copy link
Contributor Author

jttommy commented Aug 18, 2025

hey thanks for looking at those @jttommy - I would say, as smelly as that test might seem, it's giving off some smoke with this MR. I've just had a quick look on main and the allowance on it is very generous - it generally clears at around 10ms so for it to now be semi-consistently over 200ms leads me to think there's an issue here. Could you have a look please?
The wdio test you mentioned doesn't seem to ever be an issue in CI - so, yes probably a local thing.

I updated the code to only apply to @Listen and @Event decorators to limit the scope of the work to what's relevant.

@johnjenkins
UPDATE - Aug 18 2025
Tests are all passing now.

@tamb
Copy link

tamb commented Aug 27, 2025

@johnjenkins I'm totally stuck on this JSON type error. Idk what the issue is exactly. Could I get some help on it?

@jttommy jttommy marked this pull request as draft August 28, 2025 22:23
@johnjenkins
Copy link
Contributor

hey @tamb - ok will do when I get some time

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.

feat: Allow @Listen and @Event decorators to accept constants/variables for event names
3 participants