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

✨ use knitjs #4

Merged
merged 1 commit into from
Nov 29, 2016
Merged

✨ use knitjs #4

merged 1 commit into from
Nov 29, 2016

Conversation

shanewilson
Copy link

@shanewilson shanewilson commented Nov 29, 2016

uses knit instead of buildjs which means -lerna and +yarn

❯ yarn knit -- list --dependencies
yarn knit v0.17.9
$ knit list --dependencies
 ✔ discovering modules
 ✔ reading package.json of modules

info showing dependencies for 2 modules

- @oncojs/react-survivalplot (0.3.1) [3 dependencies]
└─ @oncojs/survivalplot lodash react

- @oncojs/survivalplot (0.3.3) [2 dependencies]
└─ d3 lodash

✨  Done in 0.77s.

I moved cmds into package.json so run yarn styles:build first and then yarn start. Also I added eslint but didn't fix any of the errors.

@shanewilson
Copy link
Author

@Jephuff @cheapsteak

@cheapsteak
Copy link
Contributor

Is the node_modules folder supposed to be checked in?

@cheapsteak
Copy link
Contributor

Can it be named anything besides node_modules?

@shanewilson
Copy link
Author

modules/node_modules yes so node can do package resolution without hacks

@shanewilson
Copy link
Author

you can set

package.json
{
  ...
  "knit": {
    "needle": {
      "paths": {
        "modulesStub": "whatever" 
      } 
    }
  }
}

to completely over-ride that if you want to figure out package resolution your self though

@shanewilson
Copy link
Author

> npm install
...
65.26s
❯ yarn
yarn install v0.17.9
✨  Done in 12.22s.

@shanewilson
Copy link
Author

@cheapsteak
Copy link
Contributor

I would imagine using node_modules wouldn't play well with quite a few default configs that ignore node_modules

e.g. vscode has in the editor:

  "files.watcherExclude": {
    "**/.git/objects/**": true,
    "**/node_modules/**": true
  },

// ...
  "search.exclude": {
    "**/node_modules": true,
    "**/bower_components": true
  },

Perhaps we could use linklocal in the postinstall?

@shanewilson
Copy link
Author

that is really an issue with those tools though - using a heavy handed approach to ignore instead of top level ignore which they should be doing. I get that this is the world we have to develop in, but things like linklocal are really just hacks and work arounds to what node_modules does by default.

You are certainly right that by default many configs do this wrong - even the default github .gitignore for Node ignores node_modules instead of /node_modules

@shanewilson
Copy link
Author

knit doesn't care though - since it doesn't execute the code and just does AST walking

@cheapsteak
Copy link
Contributor

Reminds me of that Kierkegaard quote about truth always resting with the minority
I usually try to go with the flow, but far be it from me to hinder anyone fighting the good fight ;)

@cheapsteak cheapsteak merged commit ad30778 into master Nov 29, 2016
@cheapsteak cheapsteak deleted the knitjs branch November 29, 2016 20:26
@shanewilson
Copy link
Author

@cheapsteak I'm hoping we won't be alone for long though:

https://gist.github.com/nolanlawson/457cdb309c9ec5b39f0d420266a9faa4
facebook/create-react-app#1081

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.

2 participants