feat: introduce meta element APIs v2 #883
Conversation
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @davidlj95 and the rest of your teammates on |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #883 +/- ##
==========================================
+ Coverage 99.45% 99.47% +0.01%
==========================================
Files 79 82 +3
Lines 368 379 +11
Branches 68 70 +2
==========================================
+ Hits 366 377 +11
Partials 2 2 ☔ View full report in Codecov by Sentry. |
📦 Bundle size (Angular v16)Git ref:
Base size data is not available yet. Try again when the CI/CD has finished running on main branch |
📦 Bundle size (Angular v15)Git ref:
Base size data is not available yet. Try again when the CI/CD has finished running on main branch |
📦 Bundle size (Angular v17)Git ref:
Base size data is not available yet. Try again when the CI/CD has finished running on main branch |
📦 Bundle size (Angular v18)Git ref:
Base size data is not available yet. Try again when the CI/CD has finished running on main branch |
davidlj95
force-pushed
the
stacked/feat-introduce-new-meta-element-apis
branch
3 times, most recently
from
6709d12 to
a506951
Compare
davidlj95
force-pushed
the
stacked/feat-introduce-new-meta-element-apis
branch
2 times, most recently
from
8db094d to
243b641
Compare
split helpers into files remove injection token based functions merge one and many into one API
davidlj95
force-pushed
the
stacked/feat-introduce-new-meta-element-apis
branch
from
243b641 to
cb8c242
Compare
|
🎉 This PR is included in version 1.0.0-beta.17 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Issue or need
Current APIs to set
<meta>elements for the page aren't very convincing. Reasons:A service is used. A lightweight token would be preferred. Also the service has just one method so even more reason for thatSee perf: injection tokens' factories don't get tree-shaken #891NgxMetaMetaDefinition. So requires an extra cognitive step before using the service.makeKeyValMetaDefinitionmakeComposedKeyValMetaDefinition<meta>can be modeled as a key / value map.<meta>elements, heck.NgxMetaDefinitionabstraction feels wrong. Better not to have a wrong abstraction. Why feels wrong?withContent(content: string)method assumes that all<meta>s will have onestringcontent value. However, for thetheme-colorstandard meta for instance, the meta accepts both amediaoptional attribute and a mandatorycontentattribute. Right now, to set thatmediaattribute, a trick is used: generate one definition on demand for each differentmedia. But doesn't sound good / defeats the purpose of the definition abstraction. Which was a factory ofMetaDefinitions<meta>elements (creating, updating, removing them) doesn't seem to need that. It only needs:attributeSelectorin case it needs to remove<meta>elements from the pageMetaDefinitionto create/update the element. Or a lack of one to actually remove the existing<meta>elements on the page.contentvalue. So doesn't support more complex cases like the one mentioned above abouttheme-colorFinally, currently there's no API to set multiple
<meta>elements with same name.That's something some metadata require though. As seen in #881.
Proposed changes
New APIs here we go. The main requirements were to fix the spots mentioned above. Taking into account:
<meta>elements identified by anameorpropertyattribute shouldn't be needed to be typed by users. i.e:name="description".<meta>elements must be allowed to be managed to solve test: standard theme color metadata setting two metas #881Use lightweight injection tokensEventually the APIs you see here were designed after several iterations:
The API to manage 1<meta>element is separated from the API to manage many<meta>s at once. This is done for tree-shaking purposes. If a user just includes metadata managers that set a single<meta>, no need for the code to manage many<meta>s.One may wonder: but why not only provide the code to manage many<meta>s and be the case of just one element array a specific case of it? Well because not sure about performance implications of removing and inserting so many nodes vs updating them instead. And if we do it this way, it's quite easy to at some later point call the many elements handler from the single element handler if there are no performance implications.Eventually, due to finding in #891 it's better to use a service. Then putting both APIs to manage one element and many elements in the same service was a better decision to avoid injecting the Angular code regarding dependency injection twice (see #112). Finally, instead of two APIs, one for 1 element and one for many elements, just 1 API to set one or many elements is provided. First to save some bytes now that we're providing a service that will come as a whole and can't provide two single-purpose APIs. Second, to provide a consistent behavior. Let's say many meta elements with same name are externally (outside of the library) added to the DOM. But then, library uses the single element setter API to update those elements. In that scenario, just first one found would be updated. Which feels weird. An API that does the same in every case is more robust.
APIs are in
@alphastate until they're used by all built-in meta modules and prove to functional.To review
Metaand use document APIs directly?After giving it a try, the refactor to do that is quite simple. However, the user may be already using
MetaAPIs. So reusing that code would result in smaller bundle size. If not using it, though, there's a win in bundle size terms. GivenMetaAPIs are provided as a service. Just 2 of them are used. But there are 7. And some code that isn't used anyway. So there would be a win in there. Well, we can care about that later.Allowing the user to choose which implementation to use would be ideal. But maybe that's going too far? To do so, the service could be turned into an abstract class / injection token. And provided a default implementation in
provideNgxMetaCore, but with the option to provide another. Though we'd loose the tree-shaking in there if unused. It's weird that this service is unused though. As setting those<meta>elements is a key piece of functionality. Also, would be difficult to tree shake the default one if providing another. As wouldn't be possible to remove the default's usage. Hmmm...Also the
DOCUMENTinjection token is another thing to couple to. Though we're coupling to it in other places. Like when setting the canonical URLlink[rel]element or modifying thehtml[lang].Will open a PR to see the bundle size changes. If it's not much, using
DOCUMENTAPIs directly decouples from usingMetaAngular API. Which is an interesting trade-off. So that the code survives deprecation ofMetaAPI.MetaDefinition? Make it compatible with it (i.e.: thehttpEquivproperty)?Makes sense in order to decouple from it. As seen in previous point. Also we make explicit that no renaming of name attributes will happen (
httpEquiv->http-equiv). Migration from aMetaDefinitionis quite straight-forward. It's also compatible withMetaDefinitionexcept from that mapped property. Also, we can add new attributes as they appear. Likemediafortheme-colorstandard metadata.API is in
@alphabut if doing so forNgxMetaElementsService.set, it disappears from docs. So using@betain there.Quick reminders