ui5-community / ui5-ecosystem-showcase

A repository showcasing the UI5 tooling extensibility to combine OSS tools for UI5 application development.
https://ui5-community.github.io/ui5-ecosystem-showcase/
Other
194 stars 93 forks source link

dev-approuter: improve routes injection and logging #1113

Closed vobu closed 2 weeks ago

vobu commented 3 weeks ago

this PR introduces two things:

petermuessig commented 3 weeks ago

@vobu - this introduces a required dependency to@sap/cds which I would avoid here. Or did you think about making it optional as the JS code suggests? In this case, please add a peerDependenciesMeta section which marks the @sap/cds dependency as optional: https://docs.npmjs.com/cli/v10/configuring-npm/package-json#peerdependenciesmeta (recommended to not get @sap/cds installed via this package). Other option would be using the optionalDependencies section but this would try to install the dependency and just in case it cannot it will not fail.

vobu commented 3 weeks ago

@vobu - this introduces a required dependency to@sap/cds which I would avoid here. Or did you think about making it optional as the JS code suggests?

yes, that was exactly the intention: use @sap/cds's logger if it's available. Will make it explicit by adding peerDependenciesMeta - again what learned, thanks for that!

petermuessig commented 3 weeks ago

@vobu: sorry for bothering you again - please fetch and rebase you change so that we can merge it - resolving the pnpm-lock.yaml is too cumbersome... THX

vobu commented 3 weeks ago

Please rebase to resolve merge conflicts

this was more ugh than I expected. But done now.

vobu commented 2 weeks ago

Please rebase to resolve merge conflicts

this was more ugh than I expected. But done now.

Hey @petermuessig, this guy https://github.com/ui5-community/ui5-ecosystem-showcase/blob/c6f7ce2a578eb1b3396a6f595b6629e20ee4784a/packages/ui5-tooling-modules/test/util.test.js#L325 is causing trouble…outdated snapshot or real error?

vobu commented 2 weeks ago

Please rebase to resolve merge conflicts

this was more ugh than I expected. But done now.

Hey @petermuessig, this guy

https://github.com/ui5-community/ui5-ecosystem-showcase/blob/c6f7ce2a578eb1b3396a6f595b6629e20ee4784a/packages/ui5-tooling-modules/test/util.test.js#L325

is causing trouble…outdated snapshot or real error?

answer-to-self: it's the snapshot file. Not DNS. But the snapshot.

petermuessig commented 2 weeks ago

When updating the dependencies of the repository, the snapshots for the ui5-tooling-modules need to be recreated. You can do this via:

pnpm --filter ui5-tooling-modules test:snapshots

But I can do so also later and finalize the PR

petermuessig commented 2 weeks ago

Ah no, I see you did it already. Ok, change is green and can be merged

nicoschoenteich commented 2 weeks ago

Sweet! Thanks a lot @vobu and @petermuessig! Glad to see the dev-approuter is still being used 🙂