dojo / core

:rocket: Dojo 2 - language helpers and utilities.
http://dojo.io
Other
213 stars 62 forks source link

tslib must be a dependency and not a devDependency #372

Closed mgroenhoff closed 6 years ago

mgroenhoff commented 6 years ago

When using importHelpers: true, tslib must be a runtime dependency, currently it is a development dependency so importing @dojo/core throws an error Error: Cannot find module 'tslib'

Pull introducing error: #367

bryanforbes commented 6 years ago

Currently, @dojo/shim is a peer dependency of @dojo/core (it must be installed as a sibling of @dojo/core) and tslib is a dependency of @dojo/shim. I just spoke with @matt-gadd and @agubler and we should probably re-evaluate what should be a peerDependency vs. a dependency vs. devDependency.

mgroenhoff commented 6 years ago

You can not assume that because tslib is a dependency of @dojo/shim that is installed next to @dojo/core, that tslib will also be accessible to @dojo/core.

Technically tslib must be a dependency of @dojo/core (because it is). You can just let NPM figure out how it dedupes and optimalizes the final output tree.

For example it is perfectly fine to have this dependency tree (which i have):

node_modules
- @dojo
    - core
       - node_modules
          -<currently tslib is missing here>
    - shim
       - node_modules
          -tslib

Using regular NPM this will probably all be flattened as much as possible and it indeed will place tslib in the root node_modules folder therefore being visible to @dojo/core because of it searching up the tree.

Peer dependencies are mostly used for regular dependencies while letting the user determine which version is used.

bryanforbes commented 6 years ago

Technically tslib must be a dependency of @dojo/core

It should be a peer dependency of @dojo/shim, which then leads it to be a peer dependency of @dojo/core.

Using regular NPM

Which version?

Peer dependencies are mostly used for regular dependencies but you want the user to choose which version he uses.

We actually don't want the user choosing which version he uses. We can assure that @dojo/* works with the version of tslib specified in the dependency of @dojo/shim. As long as it's within the 1.8.x series, the patch version doesn't matter. But @dojo/shim will not work with tslib@1.7.x. This is why tslib should be a peer dependency: so every @dojo/* package (and the user's code) will use the same tslib and then the user can't mess that up.

mgroenhoff commented 6 years ago

I thought you were talking about @dojo/core being a peer dependency of @dojo/shim. Peer dependencies also have constraints just as regular dependencies do.

It should be a peer dependency of @dojo/shim, which then leads it to be a peer dependency of @dojo/core.

You can not make assumptions like this just because you do not know where "a package manager" places them.

Consider this scenario: I am developing a package that depends on tslib@1.6.1 (one that does not work with @dojo/shim). If i use NPM to install to install the following dependencies: npm i tslib@1.6.1 @dojo/core @dojo/has @dojo/shim

  "dependencies": {
    "@dojo/core": "^0.2.1",
    "@dojo/has": "^0.1.1",
    "@dojo/shim": "^0.2.3",
    "tslib": "^1.6.1"
  }

Them npm installs it as follows:

+-- @dojo/core@0.2.1
+-- @dojo/has@0.1.1
+-- @dojo/shim@0.2.3
| +-- intersection-observer@0.4.3
| +-- pepjs@0.4.3
| `-- tslib@1.8.1
`-- tslib@1.6.1

@dojo/core will use tslib@1.6.1

mgroenhoff commented 6 years ago

Why is this labeled question. This is definitely a bug! Let me try to make it easier to understand. Please create a new directory and run the following:

$ npm i @dojo/core @dojo/has @dojo/shim --no-package-lock --global-style

$ node -e "require('@dojo/core/async/Task')"

Error: Cannot find module 'tslib'
    at Function.Module._resolveFilename (module.js:527:15)
    at Function.Module._load (module.js:476:23)
    at Module.require (module.js:568:17)
    at require (internal/module.js:11:18)
    at \node_modules\@dojo\core\async\Task.js:12:19
    at Object.defineProperty.value (\node_modules\@dojo\core\async\Task.js:3:17)
    at Object.<anonymous> (\node_modules\@dojo\core\async\Task.js:9:3)
    at Module._compile (module.js:624:30)
    at Object.Module._extensions..js (module.js:635:10)
    at Module.load (module.js:545:32)

The source code of @dojo/core contain require('tslib') statements so how could tslib not be a dependency of @dojo/core?

mgroenhoff commented 6 years ago

Do i need to make a pull request for this?

agubler commented 6 years ago

@mgroenhoff Hi, sorry for the delay getting back to you! We'd be very happy to take a pull request to make tslib a dependency of dojo/core

mgroenhoff commented 6 years ago

For reference: https://github.com/dojo/meta/issues/226