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

provide a different fair implementation to compare with in vanilla js #3

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

datner
Copy link

@datner datner commented Aug 12, 2024

addresses concerns outlined in #1

but with web-components so we can stay in class-land lmao

total LOC: 47
JS LOC: 25 (counting the <c-paged></c-paged> custom element in the html)

This example is also significantly more performant since it does not re-render the entire page, but rather only enacts atomic dom manipulations.
Its is also more scalable and safer since the usage of a shadowRoot scopes the logic, ids, styles, etc from the rest of the app

Not that any of that matters in this convoluted example, but as the name suggest, it's exemplary

EDIT: if you feel that adding data attributes is somehow cheating or changing anything significant, you can with 0 LOC change just use the indices % 5 + 1 to match instead. Not that LOC are a meaningful metric regardless

didn't even optimize for line amount
Copy link
Owner

@jbanes jbanes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I do think Web Components are the future. We just need a base library of components that's as useful as the jQuery ecosystem was. I don't think we're quite there yet.

</body>
</head>
<body>
<c-paged></c-paged>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The content tag(s) should be separated from the pager tag(s) so that they can be mixed and matched. e.g. Some applications might do this:

<c-pager for="content"></c-pager>
<c-content id="content" pages="5"></c-content>
<c-pager for="content"></c-pager>

...and others might do just this:

<c-content id="content" pages="5"></c-content>
<c-pager for="content"></c-pager>

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is an arbitrary requirement you've added post-hoc. This was never framed as component library snippet, if users want to arrange it differently, they can edit the component directly 🤨

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look up the jQuery component model. The original code represents that model. The ES6 code uses Classes for the same purpose.

You can change the original HTML exactly as I have given here. This will work fine:

<div id=“content”></div>
<div class=“pager”></div>

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also make that change in my code and it'll work fine

shadow.innerHtml = `
   <!- snip -!>
    <div class="paged-content">Page 1</div>
    <div class="pager">${pager}</div>
`

the better api would be to use slots instead of arbitrary class names and ids if you're seeking a meaningfully reusable component out of this.

The jQuery example is quite bad from a component design imho, it basically can't do components, only widgets. The difference between them as I use them here is composability. Of state, of logic, of children, of siblings, of templates. Inherently Components have all, widgets have none. You can also claim that components are declarative and widgets are imperative.
Your jQuery widget is not a component, it's unusable in a meaningful sense if you don't have control of its source directly in the editor. First of all because rendering the contents in the way you do you obliterate any contents the elements you hooked in had, and you destroy the entire view and reconstruct it on every interaction, detaching any other logic that was added. Ofc without any method to even hook into that lifecycle.

This is another reason why I used a shadow dom and a web component, since that mimics the render pattern you use without the pitfalls. You can't touch the controlled elements, and any accidental children still exist in the dom, but are not rendered (<- preserves state and so-on, failing elegantly instead of explosively)

I assume that you're very unfamiliar with web technologies, which is fine, I really don't mean that in an offensive way. Just like you'd be able to tell when some cpp abstraction is useful or helpful or required or insufficient, I am able to tell when web code is useful or helpful or required or insufficient. You probably didn't even consider your rendering approach beyond a rough estimation of a MVC/MVVM-type thing, which again, is fine, it's not your job. But these things matter, especially once you start to assert certain qualities your solution possesses which it doesn't

super();
const shadow = this.attachShadow({ mode: "open" });
let pager = "Page: ";
for (let i = 1; i <= 5; i++) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The total number of pages should be a value that can be interrogated.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By whom? For what purpose? This entire thing is self-contained. yagni.

I don't need to shove this in a state manager or a reducer for no reason, we can put this as a field on the class if you want, but theres no value

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the examples represent reusable components. They’re modeled on real world components that have seen thousands of individual uses. Remember, it’s a benchmark. Not usable code.

If YAGNI is applied, the entire project can be deleted along with Geekbench, Basemark, and other benchmarking tools. After all, they don’t do anything.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'reusable' is a significant stretch there, especially when the jQuery example isn't.. Unless by interrogated you mean "implicitly configurable on startup and can't be interrogated" and by reusable you mean that you can run the code multiple times with different arguments.

Thats now how yagni is applied and you know that... I don't mind storing these in data attributes, but you're making up new requirements on the fly. I think you should layout exactly what the requirements are, it would be more productive than raising rejections

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Serious question: Why do you say the jQuery code is not reusable?

@datner
Copy link
Author

datner commented Aug 13, 2024

FWIW, I do think Web Components are the future.

Honestly they were DOA, They have too many issues and lack critical functionality like context sharing and are constrained to working against strictly optional strings. I just used them here for fun

@Merri Merri mentioned this pull request Aug 15, 2024
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.

2 participants