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

Expose Clay's packages in the @clayui/core package #4191

Open
matuzalemsteles opened this issue Jul 28, 2021 · 14 comments · May be fixed by #5783
Open

Expose Clay's packages in the @clayui/core package #4191

matuzalemsteles opened this issue Jul 28, 2021 · 14 comments · May be fixed by #5783
Labels
3.x comp: clay-components Issues related to Clay components. (e.g ClayCard, ClayLabel...)

Comments

@matuzalemsteles
Copy link
Member

We started working on the flow of merging all of Clay's packages into one for easy maintenance, simplifying their release and updating in DXP.

#4162 introduced an initial implementation for this, adding only a few packages but we still need to add all the other packages. The purpose of this ticket is to expose the rest of these packages and also make the component types available.

@ethib137
Copy link
Member

FYI: @brianchandotcom is very interested in the progress of this ticket in order to simplify the management of Clay packages in remote apps such as this one: https://github.com/liferay/liferay-portal/blob/master/modules/apps/site-initializer/site-initializer-raylife/extra/remote-app/package.json#L20

@matuzalemsteles
Copy link
Member Author

Thanks @ethib137 for the information! We might want to prioritize this after we finish migrating the TreeView. @dsanz What do you think?

@julien
Copy link
Contributor

julien commented Feb 16, 2022

@ethib137 thanks for the heads up. @matuzalemsteles let me know if we can talk about this via Slack - @sergiojimcos and I might be able to start working on this if it's our top priority.

@dsanz
Copy link
Member

dsanz commented Feb 16, 2022

We're considering moving clay to DXP in order to make it easier for clay to reach dxp. In this effort we must consider the need for simplifying the management of dependencies. We recenlty added the concept of target platform, which applies to external developers when dealing with JS projects for liferay.

I believe we shall consider applying these ideas to the remote apps too. Just noting this to start some brainstorming, ccing @izaera as we already talked about this internally so he'd (hopefully) like to participate too ;)

@izaera
Copy link
Member

izaera commented Feb 16, 2022

We are already pulling all clay dependencies in target platforms, but that's for portlet projects (those deployed via JAR file).

In the case of remote apps, we already have a project generator that configures a build based on webpack that, when used with clay, would internalize clay deps into the generated javascript bundle.

Additionally, we are already offering the possibility to dedupe react core packages using browser modules, but that needs this PR to be merged to work.

If we manage to put all clay API into a single package (like react or angular do, for example) it would be very easy to export clay like we do with react (via browser modules) so that remote apps built with liferay-cli use the same clay the portal is using (instead of duplicating it into the webpack's generated bundle).

@ethib137 we have not tried to use liferay-cli inside liferay-portal (mainly because it is targeted at external customers), but we will need to do something similar for liferay-portal in the near future (or use liferay-cli directly), since the create_react_app.sh approach doesn't seem very scalable and maintainable for the future (at least to me).

If you want to give it a try just ping me :-)

@julien
Copy link
Contributor

julien commented Feb 16, 2022

If we manage to put all clay API into a single package (like react or angular do, for example) it would be very easy to export clay like we do with react (via browser modules) so that remote apps built with liferay-cli use the same clay the portal is using (instead of duplicating it into the webpack's generated bundle).

@izaera that's very clear and also what I had in mind - thanks. Now I just need to see if @matuzalemsteles is on the same page and I also want to know his opinion on the risks this could imply.

@izaera
Copy link
Member

izaera commented Feb 16, 2022

If we cannot export everything through the main's @clay entry point, we can also do it like now, and export several packages as browser modules. However, that's a worse option, because ideally we shouldn't have more than a handful of exported browser modules if we want to achieve good performance.

@matuzalemsteles
Copy link
Member Author

I had forgotten about this option for those who develop outside of DXP. I agree @izaera, the goal here is exactly that, keep all packages under @clayui/core, except @clayui/css, so it will be easy to deal with Clay management, it will also be easy for webpack to compile just what is being used with tree shaking, we will be exporting the components as named for this purpose.

@julien
Copy link
Contributor

julien commented Feb 16, 2022

@matuzalemsteles thanks - so we are going to literally move the files from their actual "locations" to the clay-core package, and as far as I understand that's a "breaking change" right? Does that mean it's time for 4.x?

@matuzalemsteles
Copy link
Member Author

thanks - so we are going to literally move the files from their actual "locations" to the clay-core package, and as far as I understand that's a "breaking change" right? Does that mean it's time for 4.x?

I would love to start introducing 4.x 😁 but we can do that without breaking packages, we can move all packages to @clayui/core and still keep the respective packages for each component, but they all depend on @clayui/ core. We could also try to do the opposite and just add it as a dependency in @clayui/core but we have to check that we won't have problems with circular dependency again.

@julien
Copy link
Contributor

julien commented Feb 16, 2022

I would love to start introducing 4.x grin but we can do that without breaking packages, we can move all packages to @clayui/core and still keep the respective packages for each component, but they all depend on @clayui/ core.

I see, don't you think that's adding unnecessary complexity?

We could also try to do the opposite and just add it as a dependency in @clayui/core but we have to check that we won't have problems with circular dependency again.

Exactly. My point is that this "change" is a breaking one anyway, and I think it's much easier to publish @clayui/core@4.0.0 with everything inside

@kevenleone
Copy link
Member

Hey guys, any update about this point? thanks 😄

@matuzalemsteles
Copy link
Member Author

Hey @kevenleone, unfortunately we haven't made any progress with this yet, we're just adding the new components inside @clayui/core instead of creating new packages but no plans yet to move all packages into this package.

We tried just re-exporting the components inside @clayui/core but this caused big problems with circular dependencies and the only solution to avoid this is actually moving the source code to @clayui/core and adding the @clayui/core as a dependency on each component to re-export to maintain compatibility but this still requires a lot of effort.

@matuzalemsteles
Copy link
Member Author

@matuzalemsteles matuzalemsteles linked a pull request Mar 14, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x comp: clay-components Issues related to Clay components. (e.g ClayCard, ClayLabel...)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants