-
Notifications
You must be signed in to change notification settings - Fork 504
Restore dynamic component decorators #4528
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
base: main
Are you sure you want to change the base?
Restore dynamic component decorators #4528
Conversation
…orator maps Co-authored-by: Nona Luypaert <nona.luypaert@atmire.com> Co-authored-by: Alexandre Vryghem <alexandre@atmire.com>
Co-authored-by: Alexandre Vryghem <alexandre@atmire.com>
…decorator-support_contribute-main
5c64f85 to
7952510
Compare
…hods, remove duplicate logic
…decorator-support_contribute-main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @alexandrevryghem , @artlowel ,
Many thanks for these changes. I’ve taken an initial look at the PR and performed some testing, and I wanted to share some feedback with you.
Overall, I believe this PR is an improvement over the previous static map approach, both in terms of bundle size and maintainability. It also brings back the decorator functionality, which is definitely a nice feature to have.
During my testing, however, I noticed that the script is triggered multiple times during the build process. This could be related to how files are currently being watched by the Webpack plugin, as the script appears to fire on any type of file change. This behavior could lead to unnecessary resource usage and longer build times, especially in development mode.
Additionally, the way the decorators are currently generated raises some concerns regarding the upcoming migration to Nx. As it stands, this kind of independent build process would be difficult to align with Nx’s principles—particularly the project isolation model—and might even break it.
I think it would be best to tailor this solution within the context of an Nx workspace, rethinking how decorators are built. For example, we could isolate each decorator in its own library and ensure the script rebuilds only the files within those libraries when changes occur.
In any case, I believe it would be valuable to discuss this in one of the upcoming community calls, so we can gather feedback from other developers as well.
|
Hi @alexandrevryghem, |
|
@FrancescoMolinaro Thnx for the review & sorry for the delayed response. I tested the new NX PR, alongside this script and verified that both PRs are compatible. The only thing we would need to change is that the current implementation only searches for decorators in the Regarding the isolation of the decorators in the modules, I would not recommend doing that. This would mean that if we wanted to create a new plugin for dspace that adds a To prevent services from disabled modules to be available at runtime, we could update the script in the future though to only scan for decorators inside enabled modules. Regarding performance, the script takes less than half a second to generate all the registries so I don't think this is a problem. If people would however want to disable this auto rebuild it would be easy though, you would only have to remove the plugin from webpack and run |
…decorator-support_contribute-main
References
main.jsbundle caused by standalone component migration #2899Description
When DSpace was migrated to standalone components the decorators on all the dynamic components were removed. The way dynamic components used to work was by mapping the different types of supported components with their component declaration. These mappings were automatically populated by the decorators themselves once the class was loaded, so that's why we always loaded those components eagerly. During the switch to standalone components the logic to populate the maps automatically was replaced with hardcoded maps. This PR now generates these hardcoded decorator maps automatically during every rebuild using the new
npm run generate:decorator:registriesscript, and we also lazy loaded all the component imports to reduce the main bundle size.Instructions for Reviewers
List of changes in this PR:
scripts/generate-decorator-registries.tshas been created that goes over the whole code and finds all the decorators defined inside theDECORATORSarray. These decorators are then all bundled together in a registry file that will be used to retrieve the matching component.npm run generate:decorator:registrieson every rebuildlistable-object.decorator.ts#getMatchnot always being able to fall back to a default componentlistable-object.decorator.ts#getMatchlogic in all the decorators to find the best matchGuidance for how to test this PR:
generate-decorator-registries.ts#DECORATORSand verify that they are still all rendering correctly in prod & dev modeChecklist
mainbranch of code (unless it is a backport or is fixing an issue specific to an older branch).npm run lintnpm run check-circ-deps)package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.