Remove on prefix on Angular Events

[bastiW]

Remove on prefix on Angular Events

Description

Please provide the following information:

• What changes you made: Remove on prefix on Angular Events
• Why you made them, and: Angular strictly recommends not to use prefix "on" on EventEmitters. Rule 05-16: Don't Prefix Output Properties

Make sure to follow the PR preparation steps in CONTRIBUTING.md before submitting your PR:

• format the codebase: from the root, run yarn fmt:prettier.
• update all snapshots (in core & CLI): from the root, run yarn test:update
• add Changeset entry: from the root, run yarn g:changeset and follow the CLI instructions. Alternatively, use the Changeset Github Bot to create the file.

[changeset-bot]

🦋 Changeset detected

Latest commit: 603bf04

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@builder.io/mitosis	Minor
@builder.io/mitosis-cli	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[nmerget]

You shouldn't import a function from a generator inside helpers. Right now DO_NOT_USE_VARS_TRANSFORMS is used by generators/html as well

[bastiW]

So better do that mainipulation inside

processAngularCode as part as packages/core/src/generators/angular/index.ts ?

[nmerget]

Yes do the manipulation inside packages/core/src/generators/angular/* there is a helper.ts as well

[bastiW]

@nmerget Thanks a lot for your feedback. I have some more questions:

1. I now copy pasted the code from the packages/core/src/helpers/strip-state-and-props-refs.ts to the pipe transform inside packages/core/src/generators/angular/index.ts and removed the on in the code.
   This is the easiest solution as the code has just a string in the JSON. I am thinking about better alternatives. We could make a JSON AST out of the JS Code String. Is there already an existing function for that?

2. In the Qwik E2E Test I get an error. My IDE tells me I should change

 <EventChild
        onConfirm$={$((name) => _onConfirm(props, state, name))}
        onCancel$={$(() => _onCancel(props, state))}
      ></EventChild>

to

 <EventChild
        onConfirm={$((name) => _onConfirm(props, state, name))}
        onCancel={$(() => _onCancel(props, state))}
      ></EventChild>
      

Unfortunately, I know too little about Qwik to fix it. For some reason, the route /event-listener/ is not shown in the overview when I start the Qwik project (I think this is because the qwik build failed). Furthermore, I don't know if removing the $ is correct.

[nmerget]

@nmerget Thanks a lot for your feedback. I have some more questions:

1. I now copy pasted the code from the packages/core/src/helpers/strip-state-and-props-refs.ts to the pipe transform inside packages/core/src/generators/angular/index.ts and removed the on in the code.
   This is the easiest solution as the code has just a string in the JSON. I am thinking about better alternatives. We could make a JSON AST out of the JS Code String. Is there already an existing function for that?
2. In the Qwik E2E Test I get an error. My IDE tells me I should change

 <EventChild
        onConfirm$={$((name) => _onConfirm(props, state, name))}
        onCancel$={$(() => _onCancel(props, state))}
      ></EventChild>

to

 <EventChild
        onConfirm={$((name) => _onConfirm(props, state, name))}
        onCancel={$(() => _onCancel(props, state))}
      ></EventChild>
      

Unfortunately, I know too little about Qwik to fix it. For some reason, the route /event-listener/ is not shown in the overview when I start the Qwik project (I think this is because the qwik build failed). Furthermore, I don't know if removing the $ is correct.

1. I think just copying won't be the best in this case, like it says DO_NOT_USE_VARS_TRANSFORMS. Probably the idea by using AST at some point might be the way to go. I just know that @samijaber tries to get rid of the regex replacements and wants to rely on Babel.
2. Am I wrong or are the code snippets identical? I don't know much about qwik either, sorry

[bastiW]

1. I would need a hint from @samijaber how to do that. Or we do that with a later PR.
2. IDE tells it is onConfirm= instead of onConfirm$=

[nmerget]

1. I would need a hint from @samijaber how to do that. Or we do that with a later PR.
2. IDE tells it is onConfirm= instead of onConfirm$=

Maybe this is a good hint I got in my PR:

mitosis/packages/core/src/generators/svelte/svelte.ts

Lines 150 to 185 in 664f4a1

	options.plugins = [
	...(options.plugins || []),
	processOnEventHooksPlugin(),
	FUNCTION_HACK_PLUGIN,
	// Strip types from any JS code that ends up in the template, because Svelte does not support TS code in templates.
	CODE_PROCESSOR_PLUGIN((codeType) => {
	switch (codeType) {
	case 'bindings':
	case 'properties':
	return convertTypeScriptToJS;
	case 'hooks':
	case 'hooks-deps':
	case 'state':
	case 'context-set':
	case 'dynamic-jsx-elements':
	case 'types':
	return (x) => x;
	}
	}),
	CODE_PROCESSOR_PLUGIN((codeType) => {
	switch (codeType) {
	case 'hooks':
	return flow(stripStateAndProps({ json, options }), babelTransformCode);
	case 'bindings':
	case 'hooks-deps':
	case 'state':
	return flow(stripStateAndProps({ json, options }), stripGetter);
	case 'properties':
	case 'context-set':
	return flow(stripStateAndProps({ json, options }));
	case 'dynamic-jsx-elements':
	case 'types':
	return (x) => x;
	}
	}),
	];

CODE_PROCESSOR_PLUGIN uses processOnEventHooksPlugin() which uses traverseNodes transforms the stuff before generation. I guess this could be the way to go

[bastiW]

@nmerget but the onEvent is part of

When I read the comment of component.hooks.onEvent

I see the following comments:

import {  onEvent } from '@builder.io/mitosis';

Adds an event listener to a given element ref.
  packages/ core/ src/ index. ts

It requires elementRef: T which refers to HTML Element.

So this is just for HTML refs to add event listeners, like touch, keydown... this is not the same as output emitters of the component. The output can be called from JS and must not be part of an HTML ref. So I think onEvents is not the way to go.

[nmerget]

@nmerget but the onEvent is part of

When I read the comment of component.hooks.onEvent

I see the following comments:

import {  onEvent } from '@builder.io/mitosis';

Adds an event listener to a given element ref.
  packages/ core/ src/ index. ts

It requires elementRef: T which refers to HTML Element.

So this is just for HTML refs to add event listeners, like touch, keydown... this is not the same as output emitters of the component. The output can be called from JS and must not be part of an HTML ref. So I think onEvents is not the way to go.

Sorry I think it was a little bit confusing. I tought about using something like processOnEventHooksPlugin(). So you should write your own plugin but you could do similar to processOnEventHooksPlugin() by traversing trough the nodes.