Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Move npm dependencies inside src and add CodeMirror to them #12972

Merged
merged 1 commit into from
Dec 12, 2016

Conversation

ficristo
Copy link
Collaborator

@ficristo ficristo commented Dec 7, 2016

Alternative to #12963
I was wondering if, instead of this approach, we could use some other tool to copy node_modules like using browserify, webpack or others.

Ref #12940

@zaggino
Copy link
Contributor

zaggino commented Dec 7, 2016

Yeah, definitely prefer this one over all the others. Only thing that bothers me a little bit is the generation of npm-shrinkwrap file since you need to do that from src folder AND not to forget about it 👍

@zaggino zaggino requested a review from swmitra December 7, 2016 22:40
@petetnt
Copy link
Collaborator

petetnt commented Dec 7, 2016

I'd be in for using yarn to automatically generate yarn.lock files instead npm-shrinkwrapping.

https://yarnpkg.com/en/docs/yarn-lock

@zaggino
Copy link
Contributor

zaggino commented Dec 7, 2016

That'd mean introducing another tool, we'd have to use yarn instead of npm install, not sure how yarn works with npm install scripts... to be honest I don't think yarn is worth the complexity it adds.

@petetnt
Copy link
Collaborator

petetnt commented Dec 8, 2016

I agree with introducing another tool being cumbersome, but it's a tool that's fully compatible with npm and pretty much improvement in every way (especially with speed and security), especially if we need to fiddle around with npm-shrinkwrap which is basically somewhat broken at this point.

For the grunt script the work needed would be basically s/npm install/yarn, plus with enforcing --pure-lockfile we could ensure that right deps would always be installed.

Disclaimer: I have only used yarn after it came out and I am a huge fan 😸 Not saying that we would need to do it right now (or ever), we can punt it into an issue and think about it later too 👍

@zaggino zaggino mentioned this pull request Dec 12, 2016
@zaggino
Copy link
Contributor

zaggino commented Dec 12, 2016

I think this is the best approach from the three we tried.
If we have problems with shrinkwrap, we can introduce yarn later.
For now merging this 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants