-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Move to lerna repo structure, multiple modules #1074
Conversation
@@ -33,7 +33,7 @@ | |||
"enzyme-adapter-utils": "2.9.1", | |||
"lodash": "^4.17.4", | |||
"object.values": "^1.0.4", | |||
"prop-types": "^15.5.10" | |||
"prop-types": "^16.0.0-0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's been a 16 release for prop-types
. I also don't think there's any intention to keep the prop-types
package in sync with React's versioning, so React 16 will still be using prop-types
15.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah. just figured this out :) but i'm getting a prop-types related failure in travis for react 16 only and i can't figure out why :/
99a8791
to
7a17518
Compare
7a17518
to
860341b
Compare
OK. I think I am going to merge this. This PR doesn't bring about any breaking changes and is just a reorganization of the project. I'm happy to make tweaks to this setup in the form of followup PRs, but right now I need to merge this in order to get some of the other pre-v3 PRs in. I think this structure can be greatly improved, but I think this is good enough to move forward. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding Lerna is much more than a simple reorganization of the project, and I think it needs a lot of explanation about how things work so we can all be on the same page.
None the less, I've posted a number of comments; please fix them in a followup ASAP.
- "0.12" | ||
- "0.10" | ||
before_install: | ||
- 'if [ "${TRAVIS_NODE_VERSION}" = "0.6" ]; then npm install -g npm@1.3 ; elif [ "${TRAVIS_NODE_VERSION}" != "0.9" ]; then case "$(npm --version)" in 1.*) npm install -g npm@1.4.28 ;; 2.*) npm install -g npm@2 ;; esac ; fi' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these before_install lines need to be preserved, even if we don't currently test on those node versions.
install: | ||
- 'if [ "${TRAVIS_NODE_VERSION}" = "0.6" ]; then nvm install 0.8 && npm install -g npm@1.3 && npm install -g npm@1.4.28 && npm install -g npm@2 && npm install && nvm use "${TRAVIS_NODE_VERSION}"; else npm install; fi;' | ||
before_script: | ||
- 'rm ./node_modules/.bin/npm 2>/dev/null || :' | ||
- "./node_modules/.bin/lerna bootstrap" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we npm install -g npx
first, when we could npx lerna bootstrap
here.
@@ -0,0 +1,96 @@ | |||
const path = require('path'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this file, and why do we need it?
Regardless, it must be in strict mode if it's not babel-transpiled; and ideally it should be babel-transpiled.
// This script is executed with a single argument, indicating the version of | ||
// react and adapters etc. that we want to set ourselves up for testing. | ||
// should be "14" for "enzyme-adapter-react-14", "15.4" for "enzyme-adapter-react-15.4", etc. | ||
const version = process.argv[2]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason we can't use yargs for argument parsing?
// 6. install all of the package's peer deps at the top level | ||
|
||
var root = process.cwd(); | ||
var adapterName = 'enzyme-adapter-react-' + version; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why var
and why no template literal?
"prop-types": "^15.5.10" | ||
}, | ||
"peerDependencies": { | ||
"enzyme": "2.9.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^3
@@ -0,0 +1,42 @@ | |||
{ | |||
"name": "enzyme-adapter-utils", | |||
"version": "2.9.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these versions should all start at v0.0.0 until published, when they should jump to v1.0.0.
], | ||
"author": "Leland Richardson <leland.richardson@airbnb.com>", | ||
"license": "MIT", | ||
"dependencies": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this package needs to peer-depend on enzyme as well.
{ | ||
"name": "enzyme-test-suite", | ||
"private": true, | ||
"version": "2.9.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this is private, the version is meaningless, so it should be 0.0.0
"peerDependencies": { | ||
"react": "^15.5.0" | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trailing newline
This PR moves enzyme from a single module project structure to a multiple module project structure using Lerna.
Most commands / scripts that were there before are still there and have been preserved. The modules in this project now are:
This structure was decided after testing out various configurations and running into issues with all of them except this one.
The only thing about this structure that I don't like is that I am currently running tests on our built code, rather than our source code. I tried to change this in various ways, but they all had disadvantages that I believe outweighed the advantages.