Skip to content

Conversation

arcanis
Copy link
Contributor

@arcanis arcanis commented Mar 7, 2019

What:

@babel/core is added as a peer dependency.

Why:

@emotion/styled-base has a peer dependency on @babel/core, but this package doesn't provide it - which is invalid.

Checklist:

  • Documentation - n/a
  • Tests - n/a
  • Code complete - n/a

@Andarist
Copy link
Member

Andarist commented Mar 7, 2019

Hm, this is indeed unfortunate. @emotion/styled has a dual purpose - it might be used directly as runtime library or it might be used as babel macro. Only the latter has a peer dep on @babel/core but this cannot be expressed accurately with current package.json metadata.

It would be incorrect (maybe not incorrect, but at least not developer friendly) to warn @emotion/styled users who never intend to use macro flavor about missing peer dep.

I admit this is some problem that ideally would be addressed - but I'm not sure what's the best way to fix it and provide good DX at the same time 🤔

@arcanis
Copy link
Contributor Author

arcanis commented Mar 7, 2019

Yarn supports optional peer dependencies; would that be ok if I was marking @babel/core as optional?

@Andarist
Copy link
Member

Andarist commented Mar 8, 2019

This will still warn users about missing peer dep when using managers which don't understand the meta property, right?

@arcanis
Copy link
Contributor Author

arcanis commented Mar 8, 2019

@Andarist Yep. I've tried to work with both npm and pnpm on this and while npm didn't make clear whether they would implement it or not (they considered making peer dependencies optional by default), pnpm emitted an intent to implement as well: yarnpkg/rfcs#105 (comment)

@emmatown
Copy link
Member

I'm a little confused about this, I can't see a peerDependency in @emotion/styled-base's package.json

{
"name": "@emotion/styled-base",
"version": "10.0.10",
"description": "base styled API for emotion",
"main": "dist/styled-base.cjs.js",
"module": "dist/styled-base.esm.js",
"browser": {
"./dist/styled-base.cjs.js": "./dist/styled-base.browser.cjs.js",
"./dist/styled-base.esm.js": "./dist/styled-base.browser.esm.js"
},
"types": "types/index.d.ts",
"license": "MIT",
"repository": "https://github.com/emotion-js/emotion/tree/master/packages/styled-base",
"scripts": {
"test:typescript": "dtslint types"
},
"dependencies": {
"@babel/runtime": "^7.4.3",
"@emotion/is-prop-valid": "0.7.3",
"@emotion/serialize": "^0.11.6",
"@emotion/utils": "0.11.1"
},
"devDependencies": {
"@types/react": "16.3.18",
"dtslint": "^0.3.0"
},
"peerDependencies": {
"@emotion/core": "^10.0.0",
"react": ">=16.3.0"
},
"publishConfig": {
"access": "public"
},
"files": [
"src",
"dist",
"types"
],
"umd:main": "dist/styled-base.umd.min.js",
"preconstruct": {
"umdName": "emotionStyledBase"
}
}

@Andarist
Copy link
Member

Andarist commented Nov 10, 2019

This is getting resolved in #1622 with help of optional peer dependencies. Thank you for raising awareness about this issue @arcanis !

@Andarist Andarist closed this Nov 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants