-
-
Notifications
You must be signed in to change notification settings - Fork 138
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
Prevent installation of optional peer dependencies when packaging externals #104
Comments
@olup I agree, optional peers must be skipped, but I don't think we should manage it with our packaging logic, it should be handled by
to be honest I do not really like this dirty solution, so maybe it really makes sense to change the world here and incapsulate in our packaging logic. what do you think? |
I am not sure yet, but as of now I think you are right, we should probably just implement plugins (I wanted to make a PR this week but have been delayed) and leave this as a workaround. This is a true edge-case, I think there is a questionable design from the |
Agreed, let's prioritise plugin support and if it solves the edge case then we are good |
I'll close it then. |
I'm also running into this problem :( For me particularly, JSDom has an optional peer dependency of My question is though, how is |
Hello ! Can I ask how you use JSDom in your code? Specifically, is it something you bundle as external? |
Yup, marked as external in serverless.yml esbuild:
bundle: true
minify: false
target: es2019
format: cjs
external:
- aws-lambda
- got
- keyv
- jsdom And merely having jsdom as a dependency causes Although if never importing |
Yes, that's the thing, check : #104 External are pulled with their optionals peer dependencies as of now. Plugins support will provide a workaround. But we might do something more backed in. |
We are on #104 :) Yeah I had a peek of the code and it does just pull down all peers without any condition. Simply removing that chunk did resolve it for me.... What was the rationale of grabbing all the peers like that? Seems like it'll likely cause more issues than solve |
Lol sorry, answering too early after wake up. Yes, as you read, we are thinking of some ways to fine tune this, I am not sure myself why this was added in the first place. By the way, plugin support is here so if you need a quick fix you can use that to exclude the peer dep. |
@floydspace thinking about it more, it might actually be our responsibility to not install optional peer deps in the case of externals as we are the one installing them in the first place, don't you think ? @mattisdada it's expected our our packaging to include the peer dependencies of the external modules, however I agree optionals could probably be skipped, and let user include them if needed. |
@olup you might be right, if we choose not to bundle a certain dep (with optional peers), our packager comes to the game which has to do the job right |
@mattisdada what about #113 ? Is that solving your problem ? |
While investigating #103 I saw that
pg-native
gets always bundled withpg
which might not be needed by the code (if it doesn't actually require native pg from pg). This is listed aspeerDependencies
, but set optional with :We might not want to auto bundle optional peer deps in the externals @floydspace ?
The text was updated successfully, but these errors were encountered: