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

transformTagName compatibility #59

Closed
wants to merge 16 commits into from
Closed

transformTagName compatibility #59

wants to merge 16 commits into from

Conversation

cstickel
Copy link

Changes the wrappers to be compatible to changed tag names via transformTagName option.

defineCustomElements(window, {
    transformTagName: function() {
      return 'my-prefixed-' + tagName;
    }
  });
  • For Angular: Provides an attribute selector, that there is no specific tag name required, as long as the attribute selector is used (changing tag name at runtime is not possible, is it?)
<my-prefixed-my-button my-button [someProxiedInput]="theValue">Some content</my-prefixed-my-button>
  • For React: Provides a additional factory function, that can receive a transformTagName callback to dynamically change the rendered tag name
const { MyButton: MyPrefixedMyButton } = componentsFactory((tagName: string) => {
    return `my-prefixed-${tagName}`;
  });
<MyPrefixedMyButton>Some content</MyPrefixedMyButton> // renders to <my-prefixed-my-button>Some content</my-prefixed-my-button>

@jthoms1
Copy link
Member

jthoms1 commented Jun 23, 2020

React: The introduction of the factory function means that all components will need to be bundled and used together at all times. This will essentially prevent tree shaking.

I would prefer to allow for the prefixing to occur in creation of the files rather than at runtime. Can you provide more info one why this would need to be done at runtime instead?

@jthoms1 jthoms1 self-assigned this Jun 23, 2020
@cstickel
Copy link
Author

@jthoms1 thanks for the feedback!
is there really a lot of treeshaking happening? maybe the line itself, where createReactComponent is called could be treeshaked, but that's quite just a few characters. It's mainly all the tag names and the call of the createReactComponent method. Might be a problem for really huge projects with a lot of components. Everything else is createReactComponent and it's dependencies which couldn't be treeshaked anyway, as soon as at least one component is used.
doing it at creation time would mean, that our consumers would need a dependency that creates the wrappers for them and include that as a step to their install/prebuild process, because they do use different prefixes. If we do it while runtime, we could still provide the wrappers for them.
when treeshaking really is a problem, it would also be fine to enable that just as an alterantive and keep the old generated file as it is. it will be a little bit redundant, but it's autogenerated anyway. then the library authors could choose if they prefer the treeshakable version or the one with the factory support.

@jthoms1
Copy link
Member

jthoms1 commented Jun 23, 2020

I think with how it sits right now you would be correct about the treeshaking. As we continue to introduce new ways of having interactions between stencil components and frameworks this would be less true. So keeping it as static exports on a per component level would be prefered.

With changing it as runtime I would assume that in your particular case components are not referencing each other. So 'dsn-cmp-a' would never reference 'dsn-cmp-b' or the renaming process would break some components.

@cstickel
Copy link
Author

We do have references, but also handle them dynamically inside of each component of our stencil based components library. So yes, for example p-button uses p-icon and if there is a prefix it's detecting that and dynamically prepending that to the dynamic tag name of the components it uses. To go little bit more in detail. We do have a convention, that all our components start with p- and also, that prefixes must not contain -p- or start with p-. That way we can on initialization of a component check it's own tagname and check if it starts with p- if not, everything before -p- is the prefix. so we can use it to dynamically prepend all the used internal components. That way we can have multiple versions of the component library on the same page, as long as they do have different prefixes. also it's possible to have multiple prefixes for the same version. there is some additional code that handles loading and registration of the prefixes. so that the consumers don't have to care, they just don't get into conflict with anyone else, even if we compose pages with a lot of different web components, that rely on different versions of our library.
for that reason it was also important to us, that applyPolyfills().then(() => defineCustomElements()); is not called automatically.
it's already working perfectly fine. the last missing part are the angular and react wrappers. hopefully this explanation makes the scenario more clear.

for the tree shaking.. is it possible to just generate the second file and keep the treeshakable as it is. or generate only one of them depending on the configuration, with a hint, that it will limit the treeshakability?

@jthoms1
Copy link
Member

jthoms1 commented Jun 24, 2020

I think disabling the polyfills and defineCustomElements is something that I should do.

The process for applying unique tags at runtime is probably outside of the scope of this project. I will follow up with @adamdbradley and @manucorporat to see if there is something we can do to account for it.

@jthoms1
Copy link
Member

jthoms1 commented Jul 1, 2020

I have added code to allow for disabling of polyfills and defineCustomElements in PR #62.

@jthoms1
Copy link
Member

jthoms1 commented Jul 2, 2020

I think this could all be much simpler with a configuration option. Where you pass it a function that changes the tagName or selector. Then the output code doesn't need to change and it just updates during the generation of the files. I can create a quick PR to show what I mean.

@jthoms1
Copy link
Member

jthoms1 commented Jul 6, 2020

I created a PR that should accomplish the same thing but requires fewer changes. It is #84

@cstickel
Copy link
Author

cstickel commented Jul 7, 2020

Hey Josh, thanks for the update! For angular this solution works perfectly. For react there is still no way to change at runtime, as it seems. But with the new solution we could at least add a marker to string replace later on, that could work. We'll check that.

In your new branch also both issues we experienced with 0.0.3 of angular and 0.0.7 of react are solved (angular had a wrong import and react did not respect it's default values (259d6f5#diff-482e662989a7e8fa634e455eddbbe7b8R14)).

@cstickel
Copy link
Author

cstickel commented Jul 7, 2020

This patch would readd the possiblity to add factorySyntax optional based on #84 and it's not adding much code. Would that be a possibility?

Index: packages/react-output-target/src/types.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- packages/react-output-target/src/types.ts	(revision 259d6f54fda0a702522130bcaeb5324d274be1ce)
+++ packages/react-output-target/src/types.ts	(date 1594125945051)
@@ -5,6 +5,7 @@
   loaderDir?: string;
   includePolyfills?: boolean;
   includeDefineCustomElements?: boolean;
+  useFactorySyntax?: boolean;
   // @deprecated
   tagNameModifier?: TagNameModifier;
 }
Index: packages/react-output-target/src/plugin.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- packages/react-output-target/src/plugin.ts	(revision 259d6f54fda0a702522130bcaeb5324d274be1ce)
+++ packages/react-output-target/src/plugin.ts	(date 1594125856455)
@@ -28,6 +28,7 @@
     excludeComponents: outputTarget.excludeComponents || [],
     includePolyfills: outputTarget.includePolyfills ?? true,
     includeDefineCustomElements: outputTarget.includeDefineCustomElements ?? true,
+    useFactorySyntax: outputTarget.useFactorySyntax ?? false,
     tagNameModifier: outputTarget.tagNameModifier ?? ((tagName: string) => tagName),
   };
 
Index: packages/react-output-target/src/output-react.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- packages/react-output-target/src/output-react.ts	(revision 259d6f54fda0a702522130bcaeb5324d274be1ce)
+++ packages/react-output-target/src/output-react.ts	(date 1594125881507)
@@ -65,7 +65,9 @@
     typeImports,
     sourceImports,
     registerCustomElements,
-    components.map(createComponentDefinition(outputTarget.tagNameModifier)).join('\n'),
+    outputTarget.useFactorySyntax ?
+      createComponentFactory(components) :
+      components.map(createComponentDefinition(outputTarget.tagNameModifier)).join('\n'),
   ];
 
   return final.join('\n') + '\n';
@@ -83,6 +85,25 @@
   ];
 };
 
+const createComponentFactory = (components: ComponentCompilerMeta[]) => {
+  return `export function componentsFactory(transformTagName: (tagName: string) => string = tagName => tagName) {
+  return {
+    ${components.map(createFactoryComponentDefinition()).join(',\n    ')}
+  };
+}
+`;
+}
+
+const createFactoryComponentDefinition = () => (
+  cmpMeta: ComponentCompilerMeta,
+) => {
+  const tagNameAsPascal = dashToPascalCase(cmpMeta.tagName);
+
+  return [
+    `${tagNameAsPascal}: createReactComponent<${IMPORT_TYPES}.${tagNameAsPascal}, HTML${tagNameAsPascal}Element>(transformTagName('${cmpMeta.tagName}'))`
+  ];
+};
+
 async function copyResources(config: Config, outputTarget: OutputTargetReact) {
   if (!config.sys || !config.sys.copy || !config.sys.glob) {
     throw new Error('stencil is not properly initialized at this step. Notify the developer');

…output-target

# Conflicts:
#	packages/angular-output-target/src/generate-proxies.ts
#	packages/example-project/component-library-angular/src/directives/proxies.ts
#	packages/example-project/component-library-react/src/components.ts
#	packages/react-output-target/src/output-react.ts
@jan-paulus
Copy link

Any update on this topic?

@arvindanta
Copy link

tagName

@cstickel Hi, were you able to find a way to transform tags at run time for react wrapper ?.

@trazek
Copy link

trazek commented Sep 25, 2023

Any idea why this was closed? Did this functionality make it in the core library?

@denyo
Copy link

denyo commented Sep 26, 2023

Any idea why this was closed? Did this functionality make it in the core library?

We ended up not relying on stencil-ds-output-targets anymore since this library is/was not really maintained and it seems like ever design system just forks it for their needs or builds something custom.

This pull request was closed.
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

6 participants