Skip to content
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

Merged
merged 1 commit into from
Aug 20, 2017

Conversation

lelandrichardson
Copy link
Collaborator

@lelandrichardson lelandrichardson commented Aug 16, 2017

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:

  • enzyme (the core library)
  • enzyme-adapter-react-* (each adapter is its own package)
  • enzyme-adapter-utils (shared code across the adapters)
  • enzyme-test-suite (where all of the tests live)

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.

@@ -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"
Copy link
Collaborator

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.

Copy link
Collaborator Author

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 :/

@lelandrichardson
Copy link
Collaborator Author

@aweary @ljharb build is finally passing... one of ya'll mind taking a look? I want to get this in ASAP so we are prepared to publish. The current setup is not perfect, but i think it's something we can build on...

@lelandrichardson lelandrichardson changed the title [WIP] Move to lerna repo structure, multiple modules Move to lerna repo structure, multiple modules Aug 20, 2017
@lelandrichardson
Copy link
Collaborator Author

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.

@lelandrichardson lelandrichardson merged commit c15dd47 into master Aug 20, 2017
@lelandrichardson lelandrichardson deleted the lmr--move-to-lerna branch August 20, 2017 16:29
Copy link
Member

@ljharb ljharb left a 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 [email protected] ; elif [ "${TRAVIS_NODE_VERSION}" != "0.9" ]; then case "$(npm --version)" in 1.*) npm install -g [email protected] ;; 2.*) npm install -g npm@2 ;; esac ; fi'
Copy link
Member

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 [email protected] && npm install -g [email protected] && 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"
Copy link
Member

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');
Copy link
Member

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];
Copy link
Member

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;
Copy link
Member

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",
Copy link
Member

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",
Copy link
Member

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 <[email protected]>",
"license": "MIT",
"dependencies": {
Copy link
Member

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",
Copy link
Member

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"
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trailing newline

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