Automagically install missing dependancies #306
Conversation
Add new improvements
Add all new commits
new features
DeMoorJasper
changed the title
|
Oh this is clever. |
src/utils/installPackage.js
Outdated
| module.exports = async function(dir, name) { | ||
| let yarn; | ||
| try { | ||
| yarn = await config.resolve(dir, ['yarn.lock']); |
There was a problem hiding this comment.
maybe instead of trying to detect yarn based on yarn.lock we should just look for the command?
There was a problem hiding this comment.
This sounds like a more solid approach, will change it
src/utils/installPackage.js
Outdated
| cwd: dir | ||
| }; | ||
|
|
||
| let install = spawn('yarn', ['add', name, '--dev'], options); |
There was a problem hiding this comment.
should they be dev dependencies or normal dependencies? I could see arguments for both.
There was a problem hiding this comment.
I think because it's a bundler it should always be dev deps not sure though
src/utils/localRequire.js
Outdated
| if (e.code === 'MODULE_NOT_FOUND' && triedInstall === false) { | ||
| // TODO: Make this use logger | ||
| console.log(`INSTALLING ${name}...\n`); | ||
| await install(basedir, name); |
There was a problem hiding this comment.
maybe we only want to do this for the top-level project, not for packages inside node_modules?
There was a problem hiding this comment.
Than wouldn't this still break if a dependancy isn't using a base language?
Or do you mean installing the dependancies in top-level all the time and how would this be achieved?
There was a problem hiding this comment.
but some packages like request-promise don't install by default dependencies
There was a problem hiding this comment.
How do you mean @EduardoRFS ? This PR is just for auto installing missing parsers, through changing localRequire, ofcourse this can be used for way more things in the future
DeMoorJasper
changed the title
src/utils/installPackage.js
Outdated
| const {spawn} = require('child_process'); | ||
| const config = require('./config'); | ||
|
|
||
| function testYarn(options) { |
There was a problem hiding this comment.
An alternative would be to check for a yarn.lock file in the cwd. It's probably a quicker check than spawning a whole process, and worst case we fall back to npm. This may also be desirable for the scenario that the user has yarn but the project is using npm.
There was a problem hiding this comment.
Checking for a yarn file in comparison with spawning an instance is an insane speed difference i'll pivot back towards my initial setup, thank you for your input.
451ms <-> 1ms (tested on macbook pro dual core i5)
There was a problem hiding this comment.
Ah, didn't realize you'd started with that setup!
There was a problem hiding this comment.
Wow great job!
I’m adding a blocking review to make sure that nobody merges this PR until we reach a consensus in #213. Some people are against this approach.
|
This just automatically installs dependencies not plugins, at least not yet |
|
Oh cool, didn’t realize it was a wip |
|
It's not a wip, it's just a function that can be used for installing packages instead of throwing "Module not found" Atm if a plugin is not installed it will just not parse the file the correct way, even with this merged. |
There was a problem hiding this comment.
No offense @DeMoorJasper, but when I see commit messages like stuff in a project with so much attention and hopes attached to it, it kind of breaks my heart.
|
Why use yarn instead of npm? |
|
Ok, would pick up yarn if there is a yarn.lock even it's been deprecated or regardless |
* run prettier * initial try * minor bugfixes * fix secu vuln and add yarn support * minor bugfix * cleanup output * test command instead of lockfile * pivot back to finding the package/yarn file * tiny cleanup
* run prettier * initial try * minor bugfixes * fix secu vuln and add yarn support * minor bugfix * cleanup output * test command instead of lockfile * pivot back to finding the package/yarn file * tiny cleanup
Attempt to achieve what's been discussed here: #213