teambit / envs

Component development environments for the Bit community
https://bit.dev/bit/envs
Other
63 stars 9 forks source link

Cannot find type definition file for 'lodash' #107

Open KutnerUri opened 4 years ago

KutnerUri commented 4 years ago

I am trying to tag a component which has an indirect dependency on 'lodash'.

sections/carousel-section
->
concrete/comment-carousel
->
lodash.debounce
@types/lodash.debounce ( @types/lodash ? )

I tagged concrete/comment-carousel with no problems, but when I try to tag sections/carousel-section I get this error:

Screen Shot 2020-03-17 at 16 30 24

here are my overrides:

"overrides": {
  "*": {
    "dependencies": {
      "react": "-",
      "react-dom": "-"
    },
    "devDependencies": {
      "@types/chai": "^4.2.8",
      "@types/sinon": "^7.5.1",
      "@types/mocha": "^7.0.1",
      "chai": "^4.2.0",
      "eslint-plugin-chai-friendly": "^0.5.0",
      "sinon": "^8.1.1"
    },
    "env": {
      "compiler": {
        "bit.envs/compilers/react-typescript@3.1.43": {
          "rawConfig": {
            "tsconfig": {
              "compilerOptions": {
                "target": "ES5",
                "module": "CommonJS"
              }
            }
          }
        }
      }
    }
  },
  "concrete/comment-carousel": {
    "devDependencies": {
      "@types/lodash.debounce": "+"
    }
  }
}

Specifications

KutnerUri commented 4 years ago

I got around this by adding @types/lodash.debounce to sections/carousel-section. This is not ideal, since sections/carousel-section has no direct dependency on lodash.debounce

Oddly enough, I don't need to add @types/lodash.debounce to any of the consumers of sections/carousel-section. 🤔

KutnerUri commented 4 years ago

After debugging with @qballer, we found that indirect devDependencies are not installed during build time.

This is the scenario:

CommentCarousel ->
  lodash.debounce
  @types/lodash.debounce  (dev dep)
  ...

Consumer ->
  Carousel
  chai  (dev deps)
  @types/chai (dev deps)
  ...

When building Consumer, only direct dev dependencies and recursive dependencies are installed:

// (installed components)
* CommentCarousel
  * lodash.debounce
  ...
* chai
* @types/chai
...

This is because the default behavior for npm install is not to install devDeps. Something doesn't add up for me. How are components supposed to work without their devDeps, if these are required for correct types? Do users just add @types/lodash.debounce in the consuming enviroment?

quickfix 1:

Upgrade the dependency to a 'regular' non-dev dependency. This will install correctly and successfully build.

quickfix 2:

Make the debounced method private. This removes lodash from the public API, so TS compiler skips the whole thing.

davidfirst commented 4 years ago

Thanks for the detailed info @KutnerUri !. @imsnif , I think we discussed this potential issue when implementing the "npm install" on the capsule. The way how it's working on the workspace is that Bit runs "npm install" on each one of the dependencies directories. So, no issues there. On the capsule though, if I remember correctly, it writes the dependencies paths into the package.json on the capsule's root dir and then runs only one "npm install" on that dir. Doing it this way, npm only installs the prod packages not the dev packages of the dependencies. I'm wondering whether it's possible to handle it without running "npm install" on each one of the dependencies dirs.

imsnif commented 4 years ago

Ah, cool - I don't remember we talked about it, but for sure it's the issue - nice catch @davidfirst !

I think we can probably hack our way around it by dumping all transitive devDeps into the dependent's devDeps?

Otherwise doing it in each directory will be hella slow. What do you think?

davidfirst commented 4 years ago

@imsnif , yep, that could be an option. If we do so, it'll probably be a temp package.json that will be reverted once the installation is done. (so, other processes, such as npm-pack won't use that package.json).

KutnerUri commented 4 years ago

This will be fixed by the new build flow, right?

davidfirst commented 4 years ago

Not sure. Let's keep it open to check this scenario on Harmony.