Automattic / dops-components

Shared Calypso-style components for non-Calypso projects.
GNU General Public License v2.0
9 stars 6 forks source link

Move `babel-plugin-add-module-exports` to `dependencies` #66

Closed drewblaisdell closed 8 years ago

drewblaisdell commented 8 years ago

As of #64, this package needs to be installed in every repo that transpiles dops-components or it fails when that repo (such as Delphin) tries to build anything from dops-components:

ERROR in ./~/@automattic/dops-components/client/components/gridicon/index.jsx
Module build failed: ReferenceError: Unknown plugin "add-module-exports" specified in "/Users/drewblaisdell/Desktop/a8c/delphin/node_modules/@automattic/dops-components/.babelrc" at
 0, attempted to resolve relative to "/Users/drewblaisdell/Desktop/a8c/delphin/node_modules/@automattic/dops-components"
    at /Users/drewblaisdell/Desktop/a8c/delphin/node_modules/babel-core/lib/transformation/file/options/option-manager.js:176:17
    at Array.map (native)
    at Function.normalisePlugins (/Users/drewblaisdell/Desktop/a8c/delphin/node_modules/babel-core/lib/transformation/file/options/option-manager.js:154:20)
    at OptionManager.mergeOptions (/Users/drewblaisdell/Desktop/a8c/delphin/node_modules/babel-core/lib/transformation/file/options/option-manager.js:228:36)
    at OptionManager.init (/Users/drewblaisdell/Desktop/a8c/delphin/node_modules/babel-core/lib/transformation/file/options/option-manager.js:373:12)
    at File.initOptions (/Users/drewblaisdell/Desktop/a8c/delphin/node_modules/babel-core/lib/transformation/file/index.js:221:65)
    at new File (/Users/drewblaisdell/Desktop/a8c/delphin/node_modules/babel-core/lib/transformation/file/index.js:141:24)
    at Pipeline.transform (/Users/drewblaisdell/Desktop/a8c/delphin/node_modules/babel-core/lib/transformation/pipeline.js:46:16)
    at transpile (/Users/drewblaisdell/Desktop/a8c/delphin/node_modules/babel-loader/index.js:38:20)
    at Object.module.exports (/Users/drewblaisdell/Desktop/a8c/delphin/node_modules/babel-loader/index.js:131:12)
 @ ./app/components/ui/checkout/index.js 108:15-80
 @ ./app/components/containers/checkout.js
 @ ./app/sections/checkout.js
 @ ./app/sections/index.js
 @ ./client/index.js
 @ multi app

If we move it to dependencies from devDependencies, we ensure that it will be installed. This should probably happen for other devDependencies, but the versions don't line up between this and Calypso and lining those up will require further testing.

Testing

Tug commented 8 years ago

I think it should be in peerDependencies. Updating the PR

Tug commented 8 years ago

So the main idea behind my commit is that this library should tell projects which want to use it to add those dependencies as their own.

peerDependencies are not installed by default by npm anymore so it will just show a warning:

npm WARN @automattic/dops-components@0.0.1 requires a peer of babel-plugin-add-module-exports@0.2.1 but none was installed.

Which should force devs to add this to their package.json.

There are other dependencies that could be added to peerDependencies I believe such as:

If you don't have those your build might fail. But it might just work as well, it depends on the component you're using from this lib.

So I think we should decide on a small subset of dependencies that are important for projects to have if they want to use this lib. Babel presets and webpack should definitively in the list. node-sass might be used quite a lot for an ui lib so we could have it. json-loader might be used for a couple of components, so we could ignore it...

For now let's just merge this PR which informs on babel presets and plugins which are needed :)

You can test that it works by adding #update/dependencies for this dependency in delphin's package.json:

"@automattic/dops-components": "github:automattic/dops-components#update/dependencies",
drewblaisdell commented 8 years ago

LGTM. I removed my commit.

Pinging @zinigor so that he is aware of this change. :)

Tug commented 8 years ago

👍 Let's merge