kraftvaerk / generator-rammevaerk

Scaffold a web project in kraftvaerk style
MIT License
5 stars 4 forks source link

Why we use preinstall script to install dependencies? #3

Closed kristoforsalmin closed 6 years ago

kristoforsalmin commented 6 years ago

Hi there,

I've noticed that we don't explicitly declare our dependencies, but rather install them using preinstall script. Is there any reason for that? Wouldn't it be better just to keep them under dependencies section in the package.json?

Thanks

mi2oon commented 6 years ago

@racse1 Let's see. I am using preinstall script to achieve something that was possible with the previous setup using JSPM/SystemJS.

So, what I wanted was to state the dependencies of the generator but without referencing to the actual(exact) version.

This makes it possible to lock down the dependency when someone runs the generator and install the dependencies. So, this will for instance give them the latest version of jQuery at the time of install and locking it afterwards.

However, I do need to remove the preinstall script after the generator has done it's job unless there is another way to archive this?

kristoforsalmin commented 6 years ago

@mi2oon I see, thanks. Now I got it. It's possible to do it with npm update -S too, however we still need some kind of hook to trigger that... Maybe we can replace this with:

vaerkInstall() {
    const done = this.async();

    this.installDependencies({ bower: false });
    this.spawnCommand('npm', ['update', '-S']);

    done();
}

In any case I need to test my theory first, since it could have an effect on devDependencies too.

kristoforsalmin commented 6 years ago

@mi2oon I played around with npm install -S and it works just fine. That's how it looks like:

Before

"dependencies": {
  "@fancyapps/fancybox": "latest",
  "bootstrap": "next",
  "jquery": "latest",
  "kv.cookieconsent": "github:kraftvaerk/cookie-consent"
}

After

"dependencies": {
  "@fancyapps/fancybox": "^3.1.25",
  "bootstrap": "^4.0.0-beta.2",
  "jquery": "^3.2.1",
  "kv.cookieconsent": "github:kraftvaerk/cookie-consent"
}
mi2oon commented 6 years ago

@racse1 That is great. However, it would be better if we could lock the version completely just to be safe and consistent side; So it doesn't have the ^ . It's just that not every vendor is consistent with semver and we don't want to be in the pain of having something working on our machine and not of build. It would therefore in my opinion be better to lock the version to have a near 1:1 build environment. What do you think?

Side note: ยด"kv.cookieconsent": "github:kraftvaerk/cookie-consent"ยด should also be locked like this "kv.cookieconsent": "github:kraftvaerk/cookie-consent#[version tag]" if possible.

kristoforsalmin commented 6 years ago

@mi2oon Yes, totally agree, that makes sense ๐Ÿ‘ I think I missed that you have a --save-exact flag. I'll check whether it's possible or not using npm update -S.

kristoforsalmin commented 6 years ago

@mi2oon Here's the thing I wasn't aware of. I went to the npm's website in order to check whether npm update support --save-exact flag or not and found nothing. However, if you were to go see the code, you'll discover that update command runs install under the hood, so it does support --save-exact flag ๐Ÿ˜„

Of course later I found out about npm config, but that was too easy ๐Ÿ˜„

kristoforsalmin commented 6 years ago

@mi2oon I pushed the fix, but it seems like I can't make it work for github:kraftvaerk/cookie-consent. I tried to use it like that:

"kv.cookieconsent": "github:kraftvaerk/cookie-consent#*"

But npm won't replace * with the installed version.

I can see two ways of resolving that (1) we push this plugin to npm's registry or (2) we just hardcode an actual tag instead of *. Of course we can just leave it, because it currently works the way it was before.

What do you think?

mi2oon commented 6 years ago

@racse1 Perhaps its time to refactor good old cookie consent and push to NPM. what's probably the right way. I will see if I get the time to do it, vacation coming up ;)

CookieConsent Idea: ES6 / standalone.. who is up for the challenge ๐Ÿ˜