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

Remove unused deps from extension. #140

Merged
merged 1 commit into from
Apr 5, 2016
Merged

Remove unused deps from extension. #140

merged 1 commit into from
Apr 5, 2016

Conversation

samccone
Copy link
Contributor

@samccone samccone commented Apr 5, 2016

Fixes #110

The real slowness here can be primarily blamed on npm3 not on any single dep.. soo until other things are finally resolved this is just the reality that we will have to deal with.

@brendankenny
Copy link
Member

➡️🐘⬅️

@brendankenny brendankenny added the +1 label Apr 5, 2016
@boopathi
Copy link
Contributor

boopathi commented Apr 5, 2016

The following devDependencies from extension/ can be removed as well. These will be resolved anyway from the parent_directory/node_modules.

  • gulp
  • eslint-config-google
  • and others if I've missed

basically not having duplicates in lighthouse/node_modules and lighthouse/extension/node_modules explicitly.

@boopathi
Copy link
Contributor

boopathi commented Apr 5, 2016

And since all of them are devDeps, maybe avoid declaring them in extension/package.json and declare everything in lighthouse/package.json ? - This can reduce to one time npm install from the project root.

The only problem could be the following and do we require support for this ?

npm install lighthouse-extension
npm run build

@samccone
Copy link
Contributor Author

samccone commented Apr 5, 2016

Hey @boopathi, thanks for the insight! Let me provide you with some of the motivations for the current structure.

So the idea is that someone can work on the extension in isolation from the main code. Think of it as a sub project within the main project. By requiring an isolated set of dependencies and no shared knowledge of each other from the parent or the child this sub project keeps its isolation. So depending on the parent for npm deps would violate this principle and actually make the install process slower for everyone since if we bubbled up all the deps to the parent everyone would have to install them to work on the project, vs the current situation where a user has to only install these deps when they are working on the extension.

@@ -9,9 +9,7 @@
"build": "gulp build"
},
"devDependencies": {
"babel-core": "^6.5.2",
Copy link
Member

Choose a reason for hiding this comment

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

the babel task doesnt change at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope! since we are using gulp-babel we are all set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@paulirish paulirish merged commit f693d2a into master Apr 5, 2016
@paulirish paulirish deleted the sjs/test-sketch branch April 5, 2016 22:35
@boopathi
Copy link
Contributor

boopathi commented Apr 9, 2016

@samccone But, for build or watch in extension/, you require files from parent which requires dependencies from parent anyways. So you'd have to run npm install on lighthouse/ before building lighthouse/extension

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.

Improve extension npm install time
4 participants