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

Return explicit ui5 dependencies config #851

Open
ThePlenkov opened this issue Jul 17, 2023 · 10 comments
Open

Return explicit ui5 dependencies config #851

ThePlenkov opened this issue Jul 17, 2023 · 10 comments
Labels
enhancement New feature or request

Comments

@ThePlenkov
Copy link

Hi!

I've noticed that since version 3.0 dependencies now are resolved via package.json.

I just wanted to give you the feedback that this kind of dependency collection will not work in case of monorepos.

If we use monorepo - we maintain all dependencies only on the root level. While in workspaces we use very minimalistic package.json without any dependencies ( they got automatically collected during build )

Moreover in TypeScript based scenario mapping can be resolved via tsconfig.json paths.

That's why the idea to have ui5 middleware dependencies were not bad actually and not that useless. It used probably native require.resolve() and supported all mentioned above techniques.

@ThePlenkov ThePlenkov added the enhancement New feature or request label Jul 17, 2023
@ThePlenkov
Copy link
Author

Additionally - it seems that new way of dependency resolution using ui5 workspaces also doesn't support module paths.

For example:

Error Message:
Failed to resolve workspace dependency resolution path @sap/ux-ui5-tooling to /workspaces/fiori-tools/test/@sap/ux-ui5-tooling: ENOENT: no such file or directory, open '/workspaces/fiori-tools/test/@sap/ux-ui5-tooling/package.json'

I'd suggest that we use relative path only if dot is used. Like ./@sap/ux-ui5-tooling

@RandomByte
Copy link
Member

Hey, thanks for reaching out. I think I understand your use case, but I want to make sure I also understand your setup.

I'm wondering how UI5 Tooling version 2 managed to find all the dependencies in your projects. Did you make use of the "ui5.dependencies" configuration in the package.json files of your workspaces by any chance?

If not, I'm puzzled how this could have worked.

I like the idea of allowing for module names in the workspace configuration. For now, all paths must point to directories though. Have you tried adding a workspace configuration resolving to your root project? Something like this:

specVersion: workspace/1.0
metadata:
    name: default
dependencyManagement:
    resolutions:
        - path: ../

@ThePlenkov
Copy link
Author

@RandomByte correct. I've been using ui5.dependencies in package.json. I haven't tried yet workspace - because I don't understand what is it for. Is it also for middleware resolution?

@ThePlenkov
Copy link
Author

So far my workaround is to use * versions in a package.json like here:
https://github.com/theplenkov-npm/fiori-tools/blob/main/test/package.json

while real paths defined here:
https://github.com/theplenkov-npm/fiori-tools/blob/1aa76403b4dd1d7bda89a94e0089505fd8cc6079/tsconfig.base.json#L17

and I run ui5 cli in typescript mode using this trick:

#!/usr/bin/env -S node --loader ts-node/esm
import '@ui5/cli/bin/ui5.cjs';

so it allows me to run node with ts loader and automatically *.js files got resolved to *.ts

But yes - I'd like still prefer to control this manually. Because middleware load may impact some runtime.

It's not convenient to enable/disable extensions by modifying package.json

@RandomByte
Copy link
Member

I haven't tried yet workspace - because I don't understand what is it for. Is it also for middleware resolution?

Indeed, they won't help you in this case as I just remembered myself. Workspaces only allow you to overwrite the resolution of dependencies that are already defined as a project dependency. Currently, project dependencies can only be defined by declaring an npm dependency in the corresponding package.json.

You can learn more about workspaces here: https://sap.github.io/ui5-tooling/stable/pages/Workspace/#dependency-management

So far my workaround is to use * versions in a package.json like here:

This seems to be a good workaround. Did you face any issues with this? Does npm make sure to only install the version maintained in your root package.json or would it install newer versions in the workspaces?


Ultimately, the project needs to tell UI5 Tooling which dependencies it should analyze and ultimately use in a declarative way.

Unfortunately, the "ui5.dependencies" configuration was intended as a workaround in UI5 Tooling version 2, which was not always able to analyze all npm dependencies listed in a package.json (see for example #311). Your use case to analyze dependencies not listed in the package.json was a nice side effect for your use case, but not on our radar. Therefore we removed support for this configuration with UI5 Tooling v3, as it is capable of analyzing all npm dependencies at a good performance.

I would like to refrain from re-adding this configuration option, since it also brings other side-effects and might confuse developers since it's not required anymore for most cases.

The best option I can see to support your scenario would be to make UI5 Tooling aware whenever a project is located inside an npm workspace and therefore should use all dependencies declared in the workspace root as well.

However, we will first have to check whether this is truly npm's assumption too (and Yarn's, pnpm's, etc.). I can imagine that some package managers might make the assumption that all dependencies required for a module, are exclusively defined in the module's package.json.

@ThePlenkov
Copy link
Author

Npm doesn't install any dependencies from the registry if it's a workspace package. It just creates symlink. Basically it's similar to file: dependency, just no need to care about path.
And what about middleware resolution- it is controlled by typescript of course.

@ThePlenkov
Copy link
Author

If we check how different frameworks allow extensibility:

almost everywhere plugins are maintained explicitly via config. Having dependency should not mean that this module should be imported automatically.

It's nice also that you have mentioned performance. With a straightforward declaration of middleware list it is not an issue at all, but with automatic scan indeed may appear an issue - especially when we talk about projects with long dependency list

I highly recommend to think of monorepo approach when you will control all the dependencies on a root project level, and package dependencies will be determined during build operation. You will see that in this scenario analyzing dependencies may not work

@RandomByte
Copy link
Member

For UI5 Tooling we decided that the leading source for project dependencies is the package.json file. Since we can identify relevant dependencies easily by checking for the presence of a ui5.yaml file, we wanted to keep it simple for developers and not add another configuration for that.

I can only speculate, but I think that the mentioned frameworks would have a harder time finding out which dependency is a plugin for them, and therefore have to have an explicit configuration.

Our approach requires projects to define all dependencies in the corresponding package.json. Monorepo setups are working well with this, as long as all relevant dependencies are still defined in the workspace project's package.json.

I'm not too familiar with NX, but I would be surprised if there is no mechanism for aligning those dependencies. Certainly a Monorepo can have groups of workspaces that require for example a different version of a dependency than others. How would you solve this?

Have you faced any issues with your workaround of defining the relevant dependencies in package.json using * as the version? Does this behave differently from the old "ui5.dependencies" configuration?

@ThePlenkov
Copy link
Author

No - it behaves same, but I don't like it anyway. Dependency instruction should not be used as a use instruction to my opinion. We need to enable it somewhere explicitly. Can't we introduce something like .ui5rc file with ability to extend and include plugins?

Similar to what eslintrc is doing or tsconfig

@RandomByte
Copy link
Member

Thank you very much for your feedback. I'm afraid I don't share your opinion on this matter, but I'll bring it up for discussion in our team anyways. This might take a while though due to the current holiday season.

We actually already introduced a .ui5rc file as part of https://github.com/SAP/ui5-tooling/blob/main/rfcs/0013-configuration.md. However, this only contains general (project independent) UI5 Tooling configuration.

I think if we were to add an explicit dependency declaration that overrules the package.json dependency, we should do so in the ui5.yaml.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants