search

enhance-dev / enhance-element

Enhance element base class
Apache License 2.0
14 stars 3 forks source link

chore: 🔧 improve contributing by setting up a monorepo #15

Open mariohamann opened 7 months ago

mariohamann commented 7 months ago

Hey there!

Really appreciate your work on enhance-custom-element. For me it feels a bit hard to contribute and to get together how all parts work together, as everything is distributed in different repositories which is very hard to follow as an outstander.

Do you see a chance on setting up the following repos as a monorepo?

I'm open to contribute on this issue especially if you're fine with using pnpm, which has first-class workspace support and handles versioning etc. very well. If you want to stick to npm I would have to jump into their monorepo config, which I'm not used to, but I could wrap my head around it – would just take longer. (Usually working with semantic-release and semantic-release-monorepo, but I think you're release pipeline could be integrated in a monorepo setup as well.)

brianleroux commented 7 months ago

My 0.02 is these tools assume build steps (which we deliberately avoid) and create a lot of coupling between packages (which we are also trying to avoid). Ack it seems like more work !

kristoferjoseph commented 7 months ago

I don’t see much benefit in using monorepos.

They seem to add more rigmarole than I have the time to deal with.

Each of these modules works by itself as well as together so lumping them into a single repo could also be confusing.

brianleroux commented 7 months ago

What we do have here is clearly a docs problem however. @mariohamann what sort of docs / examples would you want to see for these to help fit them together?

mariohamann commented 7 months ago

Totally get your point, and I'm very sorry, that I didn't take the appropriate time to describe my issue.

Problem

Today I worked on a web component, which had a bunch of attributes I was checking in connectedCallback and called this.render() afterwards.

I wanted to make my component more flexible to be able to use named slots without ShadowDOM and make it server-side-renderable, by extending from @enhance/custom-element. After that, everything didn't work anymore. I realized, that this.render now was already called before (!) connectedCallback.

This was irritating, as it's different from what I had in my component, from the behaviour in Lit and in Stencil and states against recommendations on MDN. But let's just have that here as a side note, maybe I will do a ticket for that somewhere else. 😄

So – I jumped into the repo of @enhance/custom-element to see the order of the mixins. Then I jumped into the different repos, and in their files. Luckliy because of your super minimal and non-build approach, it wasn't much to search for – but it took me quite some time with a bunch of browser windows open and losing my path several times.

Perspective on a monorepo

I definitely don't want to be pushy or anything. I highly appreciate your work. Feel totally free to close this issue if you want to follow your current path, I don't want to throw you into long discussions or anything – I'm a 100% sure and aware, that you have enough other stuff on the plate!

But maybe you can see it as a perspective from someone who is super interested in your project and would love to bring in things in the future – if it's lightweight to do so.

So everything I'm talking about in the following is based on my current experience + the following prototype which I built this evening: https://github.com/mariohamann/enhance What you see there is a monorepo containing most stuff besides the WASM stuff and parse-css. Everything just worked with some git commands, moving stuff into the right folders, setting up a minimal package.json and altering the pipelines a bit (didn't do anything on the release pipelines so far). (The git history is BTW completely preserved.)

  1. Coupling: On the one hand, you definitely decouple things – but still there's quite some coupling, e. g. TemplateMixin expects BaseElement (https://github.com/enhance-dev/enhance-template-mixin/blob/c732efa372f1c9fd1ebc706737b9ddf7ab6c696e/index.mjs#L5), CustomElementMixin needs this.template from TemplateMixin etc. (I even tried to use the mixins on their own (I just wanted named slots + SSR!), but just failed – I had to use everything in it's correct order.) In a monorepo you have the same coupling – but you don't have to work around with npm link or setting up branches, pointing other repos to it etc. And you can immediately search through the whole code to see where what is coming from.
  2. Testing: From me as an outstander from just seeing the code in TemplateMixin I currently have to jump to other repos to see the reason why it's needed in the constructor (and I suppose that some stuff in the end is going back to SSR). But to test if I break something, I would have to setup a bunch of repos locally, npm link them and then see what happens. I even don't know which repos I would need for that – so I usually would stop here at the latest, file an issue and call it a day. 😆 In a monorepo, you change something, run all tests and get immediate feedback if something else breaks, which you can't know without being super into the project. As an example I made this PR https://github.com/mariohamann/enhance/pull/1 which alters the position of this.render(), which passes the tests in its own package (!) TemplateMixin but breaks a lot of other packages (coupling again :)).
  3. Build steps: There's absolutely no reason to need further build steps. I didn't have to change anything in the monorepo regarding your current setup. The biggest change (besides pipelines) was this package.json with its 9 lines https://github.com/mariohamann/enhance/blob/main/package.json and updating package-lock to pickup the local workspaces – from that on it just worked.
  4. Time: Usually merging things into a monorepo can be a quite demanding task (I did it several times) – but your setup which ignores build steps as far as possible is just a beauty to work with in a monorepo, as you can ignore needed build steps more or less completely. The most complicated thing would be release handling, tagging, etc. But there are definitely ways – and I'm sure even ones, which would reflect your minimal approach.

So if you want – just play around with my little prototype with npm i, npm run test (which runs all tests at once), some cding etc.: https://github.com/mariohamann/enhance

Reach out, if you have any further questions or help is needed – and as said, feel completely free to close this issue. (Just have it maybe in your head, if you get similar feedback from others. :))

kristoferjoseph commented 7 months ago

Render gets called once in the setup without state in order to populate the template tag in order to reuse the contents as a document fragment for each instance.

Render then gets called when attributes get updated later on.

The recommendations for lit etc do not apply to Enhance Custom Elements.

The reason Lit makes sure you do not call render before connected callback is they need to do some work with your element before they allow you to. You can also create a race condition for adding event listeners if try to add them before your element is connected.

mariohamann commented 7 months ago

Thanks for the great explanation for my initial problem. 🙏 Would really love to see something of that in the docs, maybe including the information of the default lifecycle. Especially for people coming from other libraries this would be super helpful!