-
Notifications
You must be signed in to change notification settings - Fork 794
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
RFC: Deprecating dist-custom-elements-bundle #3136
Comments
I'm using dist-custom-elements-bundle but had to make few improvements in order to make it work (see #2531 (comment), few of those improvements will be useful in dist-custom-elements, e.g. external option - I will make PR for this and few others). Bundle build packs all components inside one collection into one module, which can be then imported and loaded with defineCustomElements function, how it is different to dist-custom-elements? |
@MarkChrisLevy there is no Instead, every module comes with their own
In that instance |
@johnjenkins Thanks for explanation, will give it a try and let you know if I faced any obstacles . |
In our case, SSR support is much worse with the new |
@roman-telia the team are working to improve ssr with the |
We noticed that in 2.11.0-0 there are fixes with SSR checks. We had some issues with elements using HTMLElement classes from the global scope and that caused our SSR apps to crash on the build step. We will check if the latest changes solve these issues. Regarding the Hydrate app, When we are using the hydrate app does the different bundle types have any impact on prerendered/hydrated HTML? |
I don't believe so as pre-rendered hydration is it's own output I think. Someone from the core team can correct me on this though |
Worth explicitly denoting here that plain Node SSR environments may need modules converted to a CommonJS format to work. |
I don't believe they do, however like I mentioned above, unless the environment you're using the functions produced from the dist-hydrate-script output target does not support es6 modules - like vanilla Node for example. I still need to do some research around what it would take to get the es6 modules to be read in a CommonJS format with just a plain Node server. The "dist" output target (colloquially the "Lazy load" format for your components) do have the commonjs or systemjs formats as options produced from the compiler. So we may need to consider producing those outputs as well for ease of component library consumption within Node environments for SSR. Now for the direct answer here, they generally don't, but since they remove the lazy loading features, they have more hard edges around when the component gets upgraded on the frontend, like how |
@splitinfinities @johnjenkins
In both cases I can make apropriate PR. |
@splitinfinities @johnjenkins Any comment about types and externals? (I mentioned about it in my previous comment). Those two things blocks me in making a switch from dist-custom-elements-bundle to dist-custom-elements. |
In my org we use custom-elements-bundle to then recompile the output into a systemjs bundle, which ensures we load the custom elements before the apps load (single-spa / systemjs). We basically want to eliminate lazy-loading in all instances except our own. I'm not sure if we can load it as seamlessly with the |
I tried to use If you added an index.js that wasn't empty but instead re-exported every file, we could use that to create our own bundle. |
I face exactly the same issue @MarkChrisLevy is reporting, types are not being generated |
To build on what others have said and add some (hopefully helpful?) context: We have a component library built using Stencil and we have two main groups of users -- those that want an easy plug-and-play but still performant option, e.g. they can import like so:
And a second group of micro frontend users that care a lot about optimizing performance as much as possible. They like having the ability to import individual component files and seeing zero references to unused components in their production bundle:
We were effectively serving all of our users by including all three outputTargets: dist (for the TypeScript types especially), dist-custom-elements, and dist-custom-elements-bundle in our stencil.config. In our package.json then we set the following:
This was working great and we really appreciated the flexibility the different outputTargets provided. The main pain points for us when we remove dist-custom-elements-bundle:
|
Hey all, I wrote up a quick summary of where we're at with this issue. As we start to gear up for Q1 (which starts in February for Ionic), I'll be working to help refine the scope of work needed to make the transition off of |
…tom-elements This makes a change to the `dist-custom-elements` output target code which adds exports from the generated `index.js` file for all of the components included in the build. This should help to enable Stencil users to migrate away from the `dist-custom-elements-bundle` output target, which we're planning to remove in the future (see #3136 for details and discussion on that). In order to enable this we don't need to make a particularly large change. The index chunk which we generate and pass to Rollup is just amended to include some references to the other modules we declare (one per each component), and Rollup takes care of resolving that into actual, bundled concrete modules. As part of making this change there is also a new test file added to exercise the functions for the `dist-custom-elements` output target, which necessitated improving our mocks a little bit. This should help us to test the rest of the output target code in the future. STENCIL-332 Investigate re-exporting components via index.js
…tom-elements This makes a change to the `dist-custom-elements` output target code which adds exports from the generated `index.js` file for all of the components included in the build. This should help to enable Stencil users to migrate away from the `dist-custom-elements-bundle` output target, which we're planning to remove in the future (see #3136 for details and discussion on that). In order to enable this we don't need to make a particularly large change. The index chunk which we generate and pass to Rollup is just amended to include some references to the other modules we declare (one per each component), and Rollup takes care of resolving that into actual, bundled concrete modules. As part of making this change there is also a new test file added to exercise the functions for the `dist-custom-elements` output target, which necessitated improving our mocks a little bit. This should help us to test the rest of the output target code in the future. STENCIL-332 Investigate re-exporting components via index.js
…tom-elements This makes a change to the `dist-custom-elements` output target code which adds exports from the generated `index.js` file for all of the components included in the build. This should help to enable Stencil users to migrate away from the `dist-custom-elements-bundle` output target, which we're planning to remove in the future (see #3136 for details and discussion on that). In order to enable this we don't need to make a particularly large change. The index chunk which we generate and pass to Rollup is just amended to include some references to the other modules we declare (one per each component), and Rollup takes care of resolving that into actual, bundled concrete modules. As part of making this change there is also a new test file added to exercise the functions for the `dist-custom-elements` output target, which necessitated improving our mocks a little bit. This should help us to test the rest of the output target code in the future. STENCIL-332 Investigate re-exporting components via index.js
…tom-elements This makes a change to the `dist-custom-elements` output target code which adds exports from the generated `index.js` file for all of the components included in the build. This should help to enable Stencil users to migrate away from the `dist-custom-elements-bundle` output target, which we're planning to remove in the future (see #3136 for details and discussion on that). In order to enable this we don't need to make a particularly large change. The index chunk which we generate and pass to Rollup is just amended to include some references to the other modules we declare (one per each component), and Rollup takes care of resolving that into actual, bundled concrete modules. As part of making this change there is also a new test file added to exercise the functions for the `dist-custom-elements` output target, which necessitated improving our mocks a little bit. This should help us to test the rest of the output target code in the future. STENCIL-332 Investigate re-exporting components via index.js
…tom-elements This makes a change to the `dist-custom-elements` output target code which adds exports from the generated `index.js` file for all of the components included in the build. This should help to enable Stencil users to migrate away from the `dist-custom-elements-bundle` output target, which we're planning to remove in the future (see #3136 for details and discussion on that). In order to enable this we don't need to make a particularly large change. The index chunk which we generate and pass to Rollup is just amended to include some references to the other modules we declare (one per each component), and Rollup takes care of resolving that into actual, bundled concrete modules. As part of making this change there is also a new test file added to exercise the functions for the `dist-custom-elements` output target, which necessitated improving our mocks a little bit. This should help us to test the rest of the output target code in the future. STENCIL-332 Investigate re-exporting components via index.js
…tom-elements This makes a change to the `dist-custom-elements` output target code which adds exports from the generated `index.js` file for all of the components included in the build. This should help to enable Stencil users to migrate away from the `dist-custom-elements-bundle` output target, which we're planning to remove in the future (see #3136 for details and discussion on that). In order to enable this we don't need to make a particularly large change. The index chunk which we generate and pass to Rollup is just amended to include some references to the other modules we declare (one per each component), and Rollup takes care of resolving that into actual, bundled concrete modules. As part of making this change there is also a new test file added to exercise the functions for the `dist-custom-elements` output target, which necessitated improving our mocks a little bit. This should help us to test the rest of the output target code in the future. STENCIL-332 Investigate re-exporting components via index.js
…tom-elements This makes a change to the `dist-custom-elements` output target code which adds exports from the generated `index.js` file for all of the components included in the build. This should help to enable Stencil users to migrate away from the `dist-custom-elements-bundle` output target, which we're planning to remove in the future (see #3136 for details and discussion on that). In order to enable this we don't need to make a particularly large change. The index chunk which we generate and pass to Rollup is just amended to include some references to the other modules we declare (one per each component), and Rollup takes care of resolving that into actual, bundled concrete modules. As part of making this change there is also a new test file added to exercise the functions for the `dist-custom-elements` output target, which necessitated improving our mocks a little bit. This should help us to test the rest of the output target code in the future. STENCIL-332 Investigate re-exporting components via index.js
Hey folks, We just released Stencil v2.17.0, which has the option to export your components via a single |
We are using stencil to build this library forms-reactive which is a port from angular reactive forms to any javascript. I'm trying to switch from "types": "dist/custom-elements/index.d.ts",
"module": "dist/custom-elements/index.js", I've been reading the documentation @rwaskiewicz pointed in previous comment and when switching to "types": "dist/components/index.d.ts",
"module": "dist/components/index.js", The point is that when running the build, a warning appears which is confusing me:
Please any help will be really appreciated. |
In this comment you mentioned missing the following functionality:
Can you explain your use case for this functionality? Is this specific to You mentioned that it could be achieved by a rollup plugin, is there a particular benefit that you see Stencil supporting this functionality natively? |
@rwaskiewicz Today I wanted to add a new FR related to this topic, so good timing of your comment 😉 There are two cases related to "external" option:
In the first case, lets say that we have a dependency to moment.js - right now, if you build a component and within the code of the component there are imports from moment.js, then all required code from moment.js (and its dependencies) will be put in the output bundle. That is fine and proper default behavior, but in my case moment.js is used across multiple components and within main project (which imports those components), moment.js is also directly imported so I would end up with moment.js being bundled three times. Having an "external" option in dist-custom-elements will allow to avoid that. As I mentioned, this case can be covered by a simple rollup plugin defined in stencil.config.ts for the component: export function externalsPlugin(externals: (string | RegExp)[]) {
return {
id: "externalsPlugin",
resolveId(id: string) {
for (const e of externals) {
if ((typeof e === "string" && id === e) || (e instanceof RegExp && e.exec(id))) {
return {id, external: true}
}
}
}
}
} Second case however cannot be fully resolved by a rollup plugin. Lets say, that we created a component, that uses @ionic/core lazy loaded component, e.g. ion-button. If you build the component, you will have in the output bundle the ion-button code but also other ionic components (whole collection is consumed). Having an "external" option would tell rollup to treat @ionic/core as external but also to treat stencil collections as external components. In my Stencil fork both of those cases are covered in dist-custom-elements-bundle output target, below you may see what changes I had to make in order to make it work:
Answering for your last question, is this necessary to have "external" option in Stencil - if you want to solve second case, that is a must, as it cannot be solved by a rollup plugin. But even for the first case, adding external will make my life (and likely other developers) easier and cleaner. |
Thanks @MarkChrisLevy! Would you do be a favor and create the FR(s) in that case so we can properly split them off from this issue? |
@rwaskiewicz Done - #3576 |
I'm wondering if anyone could respond to this. It would definitely be easy to migrate consumers of our component library from the deprecated custom elements to the new one if the new one also provided a I've also noticed that the docs for the non-deprecated |
@TRMW someone on the team is actively looking into this. We can't promise it would be an exact 1:1 match with the behavior of
We also have a docs revamp planned for this output target in the coming weeks 🙂 |
@rwaskiewicz This is excellent news! Looking forward to both of these updates. :) |
Hey folks, Stencil 3.0.0 was released yesterday, officially removing We're going to be doing the design work for #3576 in an upcoming sprint. In the mean time, I'm going to close this RFC out. Thank you so much for your participation/discussion! It really helped shape how v3.0.0 came out. I appreciate it 💙 |
|
Premise
In helping the Ionic Framework team the launch of v6, we've made updates to the Framework Bindings (React, Angular, and Vue) which have better bundling capabilities by using the
dist-custom-elements
output target. Through this process, we've discovered that we really didn't understand why we were maintaining two output targets -dist-custom-elements
anddist-custom-elements-bundle
.As of Stencil v2.12.0, if you are using
dist-custom-elements-bundle
, you will begin to see a deprecation message for thedist-custom-elements-bundle
output target which asks you to usedist-custom-elements
instead. In an upcoming major version of Stencil, thedist-custom-elements-bundle
will be removed. The deprecation message will have a link to this issue, so that we can keep feedback organized.If you use the
dist-custom-elements-bundle
output target, and have a unique reason that thedist-custom-elements
output target will not solve, we'd love it if you shared your use case here.Thank you so much for using Stencil!
2022.01.06 - Update
Hey folks, @rwaskiewicz here. I'm working on reviewing the needs/concerns y'all have brought up, great feedback thus far! I wanted to post a high level update in the PR summary (although I'm sure I'll have conversations with a few of you in threads).
Typings
A few of you have mentioned that typings aren't being generated for the
dist-custom-elements
output target. Although it's strange, I can confirm that that is indeed the current behavior of Stencil. I've got some additional research work scheduled around this topic, but assuming that doesn't throw up any red flags, we'll be adding typings todist-custom-elements
.ESM Support
I'll be reaching out in GH here to get a better idea of what libraries/frameworks are blocking adoption in your projects. Jest is certainly a big one that I'm aware of, but I want to get a better sense of the situation from y'all.
Externalizing Dependencies
I need to do a little more research/thinking about this before making any kind of statement on where we'll go with it
Re-exporting components via a common
index.js
fileSimilar to externalizing dependencies, I want to do a little testing around this. I don't think it would be hard per se, but I wanted to be cautious about overpromising specific solutions to (very valid) use cases.
The text was updated successfully, but these errors were encountered: