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

Femto force user to explictly install a dependencies and not have it as transitive (from npm POV) #73

Open
MangelMaxime opened this issue Jan 18, 2021 · 5 comments

Comments

@MangelMaxime
Copy link
Collaborator

I am working on binding express which have a bunch of API coming from other packages.

For example, it use mime package. I did bind the Mime API but now I am unsure if I want to add the femto properties in it.

In general, we just do npm i express but if I add the femto properties it will ask the user to install it manually in the package.json and not have it as a transitive dependencies.

Is this behaviour ok?

I am asking because in the end the user will probably have to install all the dependencies of express in it package.json which is not standard.

Should we find a way to say, Mime binding is included by Express bindings and so should not be explicitly required?

@Zaid-Ajaj
Copy link
Owner

Hello @MangelMaxime, sorry for the delayed answer 😅

I am unsure if I want to add the femto properties in it.

I don't think you need to specify mime metadata explicitly because it is already included as part of express

Should we find a way to say, Mime binding is included by Express bindings and so should not be explicitly required?

That won't be needed. Femto only installs packages that aren't already available but in case of transitive dependencies of libraries, they are there and Femto doesn't need to install anything else. Unless your binding requires a more recent version of mime that isn't included as a transitive dependency somewhere but I doubt that is your use-case 😉

@MangelMaxime
Copy link
Collaborator Author

Hello @MangelMaxime, sorry for the delayed answer 😅

Hey 👋, no problem I saw that you took a break and hope you enjoyed it :)

I don't think you need to specify mime metadata explicitly because it is already included as part of express

Indeed, we don't add the mime metadata to Fable.Express but in Fable.Mime as it's the project binding the mime package.

Should we find a way to say, Mime binding is included by Express bindings and so should not be explicitly required?

That won't be needed. Femto only installs packages that aren't already available but in case of transitive dependencies of libraries, they are there and Femto doesn't need to install anything else. Unless your binding requires a more recent version of mime that isn't included as a transitive dependency somewhere but I doubt that is your use-case 😉

I need to check again I though that Femto was reading the package.json file only and not the list of transitive package installed by npm/yarn. I will come back again to close or complete my answer depending on the situation :)

Thank you for your answer :)

@Zaid-Ajaj
Copy link
Owner

I need to check again I though that Femto was reading the package.json file only and not the list of transitive package installed by npm/yarn. I will come back again to close or complete my answer depending on the situation :)

I think you are right here. I was just reading the code and Femto does this: reads top level packages from package.json and their version range. Then reads their exact resolved versions from node_modules and doesn't look at transitive dependencies. I doubt this is a big problem because even if Femto just installs another npm package but it's already there, it shouldn't matter that much from Fable users' perspective.

We can fix this behavior but using a better way to determine the currently installed npm packages using

# using npm
npm list --json
# using yarn
yarn list --json

Then parse and flatten the results into the list of all used packages, not just the top level ones.

Do I smell the scent of a PR here? 😉

@MangelMaxime
Copy link
Collaborator Author

I doubt this is a big problem because even if Femto just installs another npm package but it's already there, it shouldn't matter that much from Fable users' perspective.

Indeed, it is not a problem from the "file system" POV (I don't have a better name for it sorry 😅).

It can be more of a problem from the user POV because he needs to manage the version of a lot more package than they are used too. In our case, the transitive dependencies instead of just express.

Do I smell the scent of a PR here? 😉

That's possible indeed 😉

@AngelMunoz
Copy link

AngelMunoz commented Mar 22, 2021

Correct me if I'm wrong but
I think this is known within the pnpm package manager https://pnpm.js.org/faq#pnpm-does-not-work-with-your-project-here
and other cli tools or templates instead of just trying to get those issues fixed in the actual offending packages (which is understandable, takes time and effort) they tend to include this (the least wanted) solution https://pnpm.js.org/faq#solution-3
which is basically to replicate the flat node modules

It can be more of a problem from the user POV because he needs to manage the version of a lot more package than they are used too

this is a real thing and that's why pnpm had to add the last resort option of adding the shamefully-hoist option

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

No branches or pull requests

3 participants