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

[refs #219] Install normalize.css as a dependency with external script #233

Closed
wants to merge 12 commits into from

Conversation

herzinger
Copy link
Contributor

@herzinger herzinger commented Nov 12, 2016

After some thinking and a lot of testing, I arrived at this solution. It covers all situations, and I tested in many different ways. By the way, the I found that this is the best way to test this sort of thing.

One little extra that I still left off just to ask about it before adding: with just some lines I can make this script work for manual downloads, without any package manager. I already know when that is the case, so I could download the dependencies via curl and move 'em to where they should be. This way we could add to the README something like "execute the bin/postinstall.sh script to download the dependencies" under the "Copy/paste (not recommended):" title.


MODE=$1
if [ -z "$MODE" ] ; then
echo -e "# manual download"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this line here, I could download all dependencies via curl. I know it wasn't executed by either npm or bower because no mode parameter was passed. Even a double click in the file would end up here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be especially useful if we start using github releases, with download files.

Copy link
Member

@anselmh anselmh left a comment

Choose a reason for hiding this comment

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

Thanks so much for investigating this further. I have a few thoughts on this:

  • Do we really need this now? I’m still not sure that we have an actual issue with our previous approach
  • Introducing a bash script into a front-end tool is very unusual, can be seen as risky and might become a risk for us when a user does something wrong. Bash scripts offer little security compared to encapsulated node.js scripts when it’s about the user’s data.
  • Introducing curl command to download something from some server is even worse since this can allow a lot of malicious things to be transferred to a user’s computer, with the risk of making the authors responsible for data loss or similar (at least accusations will be likely which would hurt the project as much)

So while this is a robust way to ensure that normalize.css is being copied to the correct location, it’s a very unsafe way to handle dependency management and I’d rather copy normalize.css manually again than introducing a bash shell script to our users.

We can’t expect from our users to use bash (some don’t), we can’t expect them to understand what this script does and if we can’t expect this, I expect that their sense of security will hopefully prevent them from using our package. Therefore, while appreciating your effort, I don’t want this script to be introduced into inuitcss’ public codebase.

This said, we have another option still left here. If we can’t solve our issues in another way, we might use this script internally to easily copy the file properly into our directory as .scss file and commit again to the repository. This would mean less effort and more reliability for authors but no impact for our users.

@florianbouvot
Copy link
Member

@anselmh 👍

@herzinger
Copy link
Contributor Author

herzinger commented Nov 13, 2016

Some considerations:

  • npm hooks ARE bash scripts. I could have written it as an one liner and dropped it in the package.json and it would work just the same. I simply moved it to it's own file, which is a best practice when using npm as a build tool (which I do and like way better than using gulp/grunt/etc, and has been becoming increasingly popular).
  • I can comment and explain each line/command in the script if it'll make you feel better. There is absolutely no security concerns with what this script does. I could even encapsulate it in a node package and publish to npm, but it would be extremely roundabout for something so simple, and also less transparent.
  • Doing this...

[...] we might use this script internally to easily copy the file properly into our directory as .scss file and commit again to the repository. This would mean less effort and more reliability for authors but no impact for our users.

...would be as simple as reverting the file and taking it out of the .gitignore. I personally see no reason for that, but I can make this change if you want.

  • @csswizardry has used similar approaches in the past, and in other repos. The curl idea came from this. In some older versions of inuitcss, it shipped with bash scripts to move and watch sass files. It was dropped with the popularization of build tools. Note: I don't mean it as a validation argument, just a curiosity.
  • As I said, npm hooks (all of them) are bash scripts. So..

We can’t expect from our users to use bash (some don’t).

...they do use it, whether they know it or not. If your concern is about they understanding or not HOW to use it, it would be as simple as double clicking the file, for users that downloaded the project manually. For anyone using bower or npm, the package manager would do everything, it would change absolutely nothing for them.

Copy link
Member

@csshugs csshugs left a comment

Choose a reason for hiding this comment

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

Also appreciating the effort being made; also thinking this is a bit too much just to copy a file from a to b.

But @anselmh brought up a good point here:

This said, we have another option still left here. If we can’t solve our issues in another way, we might use this script internally to easily copy the file properly into our directory as .scss file and commit again to the repository. This would mean less effort and more reliability for authors but no impact for our users.

Instead of running a script internally and provide the "results" in the repo, can't we run the solution proposed in #227 in our circleCI job to be sure we always deliver the most recent normalize.css version?

@csshugs
Copy link
Member

csshugs commented Dec 2, 2016

If we will handle normalize at all ourselves and not do it the way we do currently, we should do it the way suggested in #227. Hence I'm closing this.

@csshugs csshugs closed this Dec 2, 2016
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.

4 participants