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

Problem with the noInstall options #1012

Open
j0k3r opened this issue Nov 23, 2021 · 14 comments
Open

Problem with the noInstall options #1012

j0k3r opened this issue Nov 23, 2021 · 14 comments
Labels

Comments

@j0k3r
Copy link
Member

j0k3r commented Nov 23, 2021

Looks like there is an issue with the noInstall options (from #987 & #1003).
I've tested locally and it does work. I mean, the yarn install isn't launched so it's fast to package but no deps are in the package.

noInstall: false noInstall: true
the zip package 26M 617K
.webpack folder 323M 3,8M

Deps install aren't triggered but it seems that we must move the node_modules folder in proper directory if the option is enabled.
When the option is enable, there is no node_modules folder inside .webpack/dependencies/ & .webpack/service/

Can someone confirm it's broken?
poke @vicary @miguel-a-calles-mba

@j0k3r j0k3r changed the title Problem with the Problem with the noInstall options Nov 23, 2021
@j0k3r j0k3r added the bug label Nov 23, 2021
@vicary
Copy link
Member

vicary commented Nov 23, 2021

Haven't had the chance to upgrade to 5.6.0 in my projects yet.

It's midnight here, let me try bundling with the new version tomorrow.

@vicary
Copy link
Member

vicary commented Nov 24, 2021

@j0k3r See if my tests below reflects your case.

I ran the command serverless webpack an excerpt from my serverless.yml below,

package:
  individually: true
custom:
  webpack:
    packagerOptions:
      noInstall: true

In my own webpack.config.js, I did the following:

  1. A normal build yields average bundle size ~3.5MB
  2. Explicitly excluding all node_modules with nodeExternals(), average bundle size goes down to ~230KB

@j0k3r
Copy link
Member Author

j0k3r commented Nov 24, 2021

Hum, let me re-phrase.
I took a simple project, for example https://github.com/20minutes/serverless-github-check
I updated to 5.6.0.
I ran serverless package without defining noInstall (so it's false by default).
The generated zip to be sent to AWS has a size of 1,8Mo.
When I check the content of the zip I have:

" zip.vim version v31
" Browsing zipfile /Users/jeremy/Sites/github/serverless-github-check/.serverless/serverless-github-check.zip
" Select a file with cursor and press ENTER

handler.js.map
node_modules/
.............. all the node_modules folder
node_modules/jsbn/example.js
handler.js
yarn.lock
package.json

If I enabled noInstall (and nothing else), the package size went down to 115Ko.
When I check the content of the zip I have:

" zip.vim version v31
" Browsing zipfile /Users/jeremy/Sites/github/serverless-github-check/.serverless/serverless-github-check.zip
" Select a file with cursor and press ENTER

handler.js.map
handler.js
yarn.lock
package.json

Which means (for me) that deps aren't packaged when noInstall is true.

@vicary
Copy link
Member

vicary commented Nov 24, 2021

Oh, you are using the includeModules option! That maybe the difference here.

Using your repo, running rm -rf .serverless && yarn sls package && du -hs .serverless produces the following results:

  1. Using 5.5.0, or 5.6.0 with noInstall: false yields a 1.9M module.
  2. Using 5.6.0 with noInstall: true yields a 196K module.

I think this confirms the issue.

Further reading the code I found it actually made sense, because the files being copied are actually installed into .webpack/node_modules and not copied from the project root. I think noInstall should either directly copy from project root, or we simply revert because includeModules is directly depending on it.

-

In my projects I let webpack crawls into node_modules via nodeExternal({ modulesFromFile: { includeInBundle: "dependencies" } }) so it makes one single huge js bundle.

In a production environment I think it's more cleaner, without a lot of nested package.json, README.md and other unnecessary files.

If you're curious, using my method produces a bundle with 848KB.

@j0k3r
Copy link
Member Author

j0k3r commented Nov 25, 2021

Should we try to handle the case when we use both noInstall & includeModules or just drop the noInstall option?

@vicary
Copy link
Member

vicary commented Nov 25, 2021

Using noInstall with includeModules make no sense right now, as an immediate patch I think we should throw when both options are true.

I am not sure what the original feature request wants to achieve, maybe invite the original author for clarifications?

Just drop the option in the next major upgrade if we get no replies until then.

@vicary
Copy link
Member

vicary commented Nov 25, 2021

@russell-dot-js I think we need your input here. In #913, do you want to pack the node_modules from project root instead of the throw-away directory at .webpack/node_modules?

@j0k3r
Copy link
Member Author

j0k3r commented Dec 2, 2021

@russell-dot-js could you please share your serverless.yml configuration you are using with the noInstall option? Just to check how is it working on your side? Thanks

@micthiesen
Copy link

For my project being able to use the node_modules from the project root (with noInstall: true and includeModules: ...) would be useful. We're using patch-package to patch a few problematic packages but they're only patched in the project's node_modules. So unless we can use the project's node_modules we'll be using un-patched packages.

Any ideas for workarounds would be appreciated too.

@russell-dot-js
Copy link
Contributor

Hi all,

Sorry for the late reply. You are correct - we always run includeModules: false and I never tested noInstall + includeModules. We minify our modules and bundle them with webpack (and think tree shaking makes it worth it for you to do so too!)

noInstall was mainly introduced to solve for yarn PNP. I would suggest not using it if you aren't minifying your node modules and including them in your bundle.

@njenwei
Copy link

njenwei commented May 19, 2022

This combination works for me, running webpack in a workspace within an npm monorepo. Each lambda is only packaged with its required dependencies, packed into a single file. Webpack is able to search for dependencies on the root level as well as local packages in other workspaces.

    includeModules: false
    packagerOptions:
      noInstall: true

With this, a node_modules folder is created and all lambda share a single aggregated package.json, causing the built artifact to be quite large:

    includeModules:
      nodeModulesRelativeDir: '../../'
    packagerOptions:
      noInstall: false

@vicary
Copy link
Member

vicary commented Sep 6, 2022

@njenwei Please share your webpack config.

If you are already letting webpack crawls into your node_module directory, i.e. without nodeExternals(), you need neither includeModules nor noInstall.

@arturenault
Copy link

@russell-dot-js I think we need your input here. In #913, do you want to pack the node_modules from project root instead of the throw-away directory at .webpack/node_modules?

Coming in here super late, I would love this feature.
Currently the throwaway directory doesn't work for yarn workspaces (at least with noFrozenLockfile: false) because yarn install --frozen-lockfile sees a huge yarn.lock and a tiny package.json, decides the lockfile needs to be updated and throws an error.

Copying packages from project root would avoid this issue (and speed up our builds massively)

@vicary
Copy link
Member

vicary commented Jan 29, 2023

@arturenault It was a different context. With noInstall webpack may grab all packages in project root, that's usually more than you need.

It's the packagers' job to decide if it wants to copy or even symlink, if it's exists in .yarn/cache it may copy.

If you are using berry, you may try these:

  1. Add nmHoistingLimits: dependencies in .yarnrc.yml at your project root
  2. Add a new workspace pattern to include the throwaway directory

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

No branches or pull requests

6 participants