feat(angular-output-target): outputType configuration #365

sean-perkins · 2023-07-20T19:57:33Z

Pull request checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been reviewed and added / updated if needed (for bug fixes / features)
Build (npm run build) was run locally for affected output targets
Tests (npm test) were run locally and passed
Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

What is the current behavior?

N/A

Issue URL: Internal

What is the new behavior?

This PR is the first phase of work for adding standalone component generation to the Angular output target.

Adds new outputType option to the Angular output target for specifying the desired type of generated output.
Removes includeSingleComponentAngularModules option in favor of outputType: "scam".
Removes includeImportCustomElements option in favor of providing a value for customElementsDir.
- As a result, the Angular output target will no longer default to the components directory if not providing a customElementsDir.

Does this introduce a breaking change?

Yes
No

Developers using the lazy/hydrated output should specify outputType: "component" in their Stencil config.
includeSingleComponentAngularModules is removed, use outputType: "scam" instead.
includeImportCustomElements option is removed, define a path for customElementsDir. Our recommended path is components.

Other information

liamdebeasi · 2023-07-25T13:29:15Z

packages/angular-output-target/src/plugin.ts

@@ -42,11 +42,5 @@ export function normalizeOutputTarget(config: Config, outputTarget: OutputTarget
    results.directivesArrayFile = normalizePath(path.join(config.rootDir, outputTarget.directivesArrayFile));
  }

-  if (outputTarget.includeSingleComponentAngularModules !== undefined) {


Should this still be considered experimental?

I don't know at this point. It felt strange that we are adding a new output type that isn't experimental, but keeping the scam generation as experimental.

While Angular is pushing hard on the ecosystem to lean towards standalone, existing applications and large companies just aren't going to move in that direction quickly or at all. The need for maintaining the feature will exist for a few years at least, which seems like a long time to keep it in an experimental stage.

I'd be happy to revert my changes at this part of work (keeping scam as experimental) and then open a design doc for discussion to move it out of experimental. Feels the most appropriate with our process 👍

I agree with the points you brought up regarding SCAMs. I think reverting the change + a design document makes sense here since we can get some cross-team discussions going.

Addressed here: a2ff7a7

Also add an error for developers currently using includeSingleComponentAngularModules option to migrate to outputType (seemed like a minimal DX touch to help reduce confusion).

liamdebeasi · 2023-07-25T13:32:16Z

packages/angular-output-target/README.md

+| `directivesArrayFile`  | The output file of a constant of all the generated component wrapper classes. Used for easily declaring and exporting the generated components from an `NgModule`. This file path should point to a location within your Angular library/project.                                       |
+| `valueAccessorConfigs` | The configuration object for how individual web components behave with Angular control value accessors.                                                                                                                                                                                 |
+| `excludeComponents`    | An array of tag names to exclude from generating component wrappers for. This is helpful when have a custom framework implementation of a specific component or need to extend the base component wrapper behavior.                                                                     |
+| `outputType`           | The type of output to generate. If `component`, the output target will generate lazy loaded component wrappers tied to an Angular module. If `scam`, the output target will generate a single component angular module for each component. If `standalone`, standalone component wrappers will be generated. |


The type of output to generate. If component, the output target will generate lazy loaded component

This isn't necessarily true since you can have the following, which would using dist-custom-elements with a single angular module.

angularOutputTarget({ componentCorePackage: '@ionic/core', directivesProxyFile: '../angular/src/lazy-loaded/directives/proxies.ts', customElementsDir: 'components', // You can add outputType: 'component', but since `component` is the default it's not required. });

Yeah I did not word this right at all..

Developers should only use the outputType: "component" when using the hydrated bundle. Otherwise they should choose between outputType: "scam" or outputType: "standalone" if using the custom elements bundle.

Addressed here: 62baec2

The changes look good. Should we add stronger verbiage around customElementsDir being required for scam/standalone? Right now scam/standalone is recommended for dist-custom-elements, but we don't state that if you want to use scam/standalone you must use dist-custom-elements.

Good thought, it really isn't "recommended", it is required. I'll adjust after stand-up with some stronger language around the requirements.

Here's another attempt: 1e987c2

packages/angular-output-target/src/output-angular.ts

packages/angular-output-target/src/types.ts

rwaskiewicz

One non-blocking README suggestion, and one question/thought for the future

packages/angular-output-target/README.md

rwaskiewicz · 2023-07-26T13:57:28Z

packages/angular-output-target/README.md

+| `valueAccessorConfigs` | The configuration object for how individual web components behave with Angular control value accessors.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                  |
+| `excludeComponents`    | An array of tag names to exclude from generating component wrappers for. This is helpful when have a custom framework implementation of a specific component or need to extend the base component wrapper behavior.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                      |
+| `outputType`           | Specifies the type of output to be generated. It can take one of the following values: <br />1. `component`: Generates all the component wrappers to be declared on an Angular module. This option is required for Stencil projects using the `dist` hydrated output.<br /> 2. `scam`: Generates a separate Angular module for each component.<br /> 3. `standalone`: Generates standalone component wrappers.<br /> Both `scam` and `standalone` options are compatible with the `dist-custom-elements` output. Developers **must** set a `customElementsDir` in the output target config.<br />Note: Please choose the appropriate `outputType` based on your project's requirements and the desired output structure. |
+| `customElementsDir`    | This is the directory where the custom elements are imported from when using the [Custom Elements Bundle](https://stenciljs.com/docs/custom-elements). Required to be set for `outputType: "scam"` or `outputType: "standalone"`.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                        |


And....there's my answer 😆

rwaskiewicz · 2023-07-26T14:09:49Z

packages/angular-output-target/src/types.ts

+ * - `scam` - Generate a Single Component Angular Module for each component.
+ * - `standalone` - Generate a component with the `standalone` flag set to `true`.
+ */
+export type OutputType = 'component' | 'scam' | 'standalone';


I wonder if in the future, it would make sense to group necessary configuration fields together. For example, if for 'scam' and 'standalone' we require 'customElementsDir' to be set (and conversely, don't do anything with it for 'component' (IIUC, which may not be the case), we can use TypeScript to guide folks towards a more correct configuration:

type ComponentConfiguration = { type: 'component', } type ScamConfiguration = { type: 'scam', customElementsDir: 'string', } type StandalongConfiguration = { type: 'standalone', customElementsDir: 'string', } export type OutputType = ComponentConfiguration | ScamConfiguration | StandaloneConfigration;

I like that suggestion a lot. Currently the output targets are a lot of top-level configurations, but have mixed dependencies on each other that validate at runtime. It would be great if the configuration was from the perspective of what you want to generate for that framework (e.g. Angular library authors likely know if they want to generate standalone components, SCAM or just a bunch of components on a single module) and let typescript assist with the required fields for that configuration.

It also removes the need for runtime checks (if we wanted to move away from that approach).

👍 FWIW, I think we'd still need the runtime checks - Stencil config type checks only occur within an editor that is running the TS Language Server, but not at compile time. If you're editing the Stencil config in say, TextEdit on a Mac, you wouldn't get any feedback if you did something like:

{ type: 'the_cheese_standalone', }

Agree with this. Kinda what I was trying to do here

Co-authored-by: Ryan Waskiewicz <[email protected]>

Co-authored-by: Liam DeBeasi <[email protected]>

feat(angular-output-target): outputType configuration

41b86fe

sean-perkins requested review from a team as code owners July 20, 2023 19:57

sean-perkins requested review from rwaskiewicz, alicewriteswrongs and liamdebeasi and removed request for a team July 20, 2023 19:57

sean-perkins added 2 commits July 20, 2023 15:58

chore: update readme

57d0321

chore: update default code snippet

922524a

sean-perkins mentioned this pull request Jul 21, 2023

docs(angular): outputType API for standalone components ionic-team/stencil-site#1183

Merged

liamdebeasi reviewed Jul 25, 2023

View reviewed changes

packages/angular-output-target/src/output-angular.ts Outdated Show resolved Hide resolved

sean-perkins added 4 commits July 25, 2023 11:15

chore: clarify readme usage

62baec2

fix: runtime check for standalone components with customElementsDir

36eb24a

chore: rephrase outputType and customElementsDir option

1e987c2

chore: re-add experimental warning, add migration message

a2ff7a7

liamdebeasi approved these changes Jul 25, 2023

View reviewed changes

packages/angular-output-target/src/types.ts Outdated Show resolved Hide resolved

rwaskiewicz self-assigned this Jul 26, 2023

rwaskiewicz approved these changes Jul 26, 2023

View reviewed changes

sean-perkins and others added 3 commits July 26, 2023 11:06

chore: update readme description for outputType

6c0ee6b

Co-authored-by: Ryan Waskiewicz <[email protected]>

feat(angular-output-target): generate standalone components (#366)

cb5e32f

chore: make scam generation experimental

bf000ac

Co-authored-by: Liam DeBeasi <[email protected]>

sean-perkins merged commit 5b11e95 into sp/FW-4756 Jul 27, 2023
3 checks passed

sean-perkins deleted the sp/FW-4756-output-type branch July 27, 2023 16:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(angular-output-target): outputType configuration #365

feat(angular-output-target): outputType configuration #365

sean-perkins commented Jul 20, 2023

liamdebeasi Jul 25, 2023

sean-perkins Jul 25, 2023

liamdebeasi Jul 25, 2023

sean-perkins Jul 25, 2023

liamdebeasi Jul 25, 2023

sean-perkins Jul 25, 2023 •

edited

Loading

sean-perkins Jul 25, 2023

liamdebeasi Jul 25, 2023 •

edited

Loading

sean-perkins Jul 25, 2023

sean-perkins Jul 25, 2023

rwaskiewicz left a comment

rwaskiewicz Jul 26, 2023

rwaskiewicz Jul 26, 2023

sean-perkins Jul 26, 2023

rwaskiewicz Jul 26, 2023

tanner-reits Jul 26, 2023

feat(angular-output-target): outputType configuration #365

feat(angular-output-target): outputType configuration #365

Conversation

sean-perkins commented Jul 20, 2023

Pull request checklist

Pull request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Other information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sean-perkins Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamdebeasi Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwaskiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sean-perkins Jul 25, 2023 •

edited

Loading

liamdebeasi Jul 25, 2023 •

edited

Loading