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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Breaking: Use typed event registration and dispatch #1397

Closed
wants to merge 14 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@antross
Member

antross commented Oct 15, 2018

Pull request checklist

Make sure you:

For non-trivial changes, please make sure you also:

  • Added/Updated related documentation.
  • Added/Updated related tests.

Short description of the change(s)

Fix #123. No change is required for hints which consume only the
core set of events (fetch::*, element::*, scan::*, traverse::*).

Hints which consume parser-specific events such as
parse::manifest::end now need to import the events for that parser
and apply them to their HintContext:

import { ManifestEvents } from `@hint/parser-manifest`;

public constructor(context: HintContext<ManifestEvents>) {
    ...
    context.on('parse::manifest::end', checkManifest);
}

Similarly, new Parsers need to explicitly declare and export added
events so they can be imported and consumed elsewhere:

import { Events } from `hint/dist/src/lib/types`;
...
export type ManifestEvents = Events & {
    'fetch:馃敋:manifest': FetchEnd;
    'fetch::error::manifest': FetchError;
    'fetch::start::manifest': FetchStart;
    'parse::manifest::end': ManifestParsed;
    'parse::manifest::error::schema': ManifestInvalidSchema;
    'parse::manifest::error::json': ManifestInvalidJSON;
};
@antross

This comment has been minimized.

Member

antross commented Oct 15, 2018

Core work is complete, marked work-in-progress while I check for test failures and update documentation.

@antross

This comment has been minimized.

Member

antross commented Oct 15, 2018

@alrra and @molant: Given this is already a breaking change, do we want to take this opportunity to normalize how fetch and parse events are represented? Currently we have fetch::end::<resource-type> and parse::<resource-type>::end. Personally I prefer putting the variable part of the event name at the end as in the fetch events, but we also generate events such as parse::<resource-type>::error::schema which may need to become parse::error::<resource-type>::schema for predictability if we take this approach. Thoughts?

On a related note, only parser-babel-config and parser-typescript-config emit a parse::<resource-type>::start event - all other parsers only emit parse::<resource-type>::end or parse::<resource-type>::error::* events. Granted, these two both trigger from a fetch::end::json event whereas other parsers have a uniquely fetched resource type, but it still feels a bit inconsistent. Should we add a parse::<resource-type>::start to the other parsers for consistency?

@antross

This comment has been minimized.

Member

antross commented Oct 15, 2018

Also, I don't see the README files for most of our parsers showing up as documentation on webhint.io. Am I missing something or do these need to be added as links from the events page in the contributor guide? I can do so as part of this PR if someone tells me how to reference them.

I also expected them to be listed on the parsers page in the user guide, but only the javascript and typescript-config parsers appear there and those links are broken. I'll plan to update this page as well once we figure out where/how to link to the appropriate information.

@molant

This comment has been minimized.

Member

molant commented Oct 15, 2018

@antross the documentation errors could be related to webhintio/webhint.io#523 and/or webhintio/webhint.io#520

@sarvaje

LGTM, I just have some questions.

Show resolved Hide resolved packages/formatter-excel/src/formatter.ts Outdated
Show resolved Hide resolved packages/hint/src/lib/engine.ts
Show resolved Hide resolved packages/hint/src/lib/engine.ts
Show resolved Hide resolved packages/hint/src/lib/engine.ts
@antross

This comment has been minimized.

Member

antross commented Oct 15, 2018

the documentation errors could be related to webhintio/webhint.io#523 and/or webhintio/webhint.io#520

@molant that doesn't appear to be the case here. The broken links are pointing to npmjs.com and just need their path adjusted (to use package/ instead of packages/) which I can do, but I thought we might prefer to point to our own rendering if we can figure out the link structure (I don't see any examples of cross-package linking currently). The rest are simply missing from the documentation and not linked at all.

Also, any thoughts on my earlier comment about re-ordering the components of the parse::* events to align with the fetch::* events?

@molant

This comment has been minimized.

Member

molant commented Oct 15, 2018

I've synced with @antross over phone regarding the event naming.

What we agreed is to keep them as consistent as possible so we will have parse::start::XXX and parse::end:XXX. Also @antross will be adding the missing parse::start events.

Remember to update the create-parser project 馃槉

@@ -236,5 +225,6 @@ type CanEvaluateScript {
<!-- Link labels: -->
[nodeName docs]: https://developer.mozilla.org/en-US/docs/Web/API/Node/nodeName
[parsers]: https://webhint.io/docs/user-guide/concepts/parsers/
[postcss]: https://postcss.org/

This comment has been minimized.

@sarvaje

sarvaje Oct 15, 2018

Member

can you double check if this link is still used in this file? It looks like it was part of the parse::css section.

This comment has been minimized.

@antross

antross Oct 16, 2018

Member

Good catch, thanks!

@antross antross self-assigned this Oct 18, 2018

@alrra alrra force-pushed the webhintio:master branch 2 times, most recently from 7589858 to 3971182 Oct 31, 2018

antross added some commits Oct 13, 2018

Chore: Make `target` optional for `formatter` calls
Type-change only. Aligns type-validation with existing behavior.
Breaking: Use typed event registration and dispatch
Fix #123. No change is required for hints which consume only the
core set of events (`fetch::*`, `element::*`, `scan::*`, `traverse::*`).

Hints which consume parser-specific events such as
`parse::manifest::end` now need to import the events for that parser
and apply them to their `HintContext`:

```typescript
import { ManifestEvents } from `@hint/parser-manifest`;

public constructor(context: HintContext<ManifestEvents>) {
    ...
    context.on('parse::manifest::end', checkManifest);
}
```

Similarly, new `Parser`s need to explicitly declare and export added
events so they can be imported and consumed elsewhere:

```typescript
import { Events } from `hint/dist/src/lib/types`;
...
export type ManifestEvents = Events & {
    'fetch:馃敋:manifest': FetchEnd;
    'fetch::error::manifest': FetchError;
    'fetch::start::manifest': FetchStart;
    'parse::manifest::end': ManifestParsed;
    'parse::manifest::error::schema': ManifestInvalidSchema;
    'parse::manifest::error::json': ManifestInvalidJSON;
};
```
Fix: Use typed event registration
Refs #123. Fixes affected hints and connectors to work with changes
introduced by ef41732.
Chore: Fix test failure
Forgot to update validation after changing the emitted event
earlier in this test.
Chore: Remove unnecessary `eslint-disable` directives
These were causing linting to fail.
Chore: Bulk-convert `Array<type>` to `type[]`
Only a few `Array<>` references remain where the contained type
was complex enough that the `Array<>` form proved more legible.
Breaking: Rename `parse::*::end`, etc. to `parse:馃敋:*`
For consistency with `fetch` event names. Includes related updates
to documentation.
Chore: Fix parser tests
Need to account for the new `parse::start::*` event.

@antross antross force-pushed the antross:typed-events branch from eea80e1 to 1dd96be Nov 1, 2018

@antross

This comment has been minimized.

Member

antross commented Nov 1, 2018

@alrra @molant This is now up-to-date when you have time to take a look. I ran into some intermittent CI failures, but it looks like the current code should pass on re-run (at least where canvas-prebuilt works).

@molant

This comment has been minimized.

Member

molant commented Nov 1, 2018

the current code should pass on re-run (at least where canvas-prebuilt works).

There's already versions for Linux and Mac. Don't know why it's still failing but I'm sure it's unrelated to your changes :(

@molant

molant approved these changes Nov 1, 2018

Show resolved Hide resolved packages/hint/src/lib/types/event.ts Outdated
Chore: Apply PR feedback
Co-Authored-By: antross <antross@gmail.com>

@alrra alrra closed this in 8499d5c Nov 2, 2018

@antross antross deleted the antross:typed-events branch Nov 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment