Skip to content

Conversation

@brettz9
Copy link
Contributor

@brettz9 brettz9 commented Jun 6, 2018

  • Linting: Add browser and node_modules to .eslintignore
  • npm: Simplify test script

@whitlockjc
Copy link
Owner

I'm already running eslint as part of gulp. Is this necessary?

@brettz9
Copy link
Contributor Author

brettz9 commented Jun 8, 2018

Whoops... Sorry about that...

However, the PR should still be useful as .eslintignore is useful to IDE linting plugins to avoid triggering errors and warnings when browsing a file in the repo that is not supposed to have the linting rules applied.

I also simplified the package.json test script which doesn't need to hard-code the path to the gulp binary given that dependencies are added to the path (see https://docs.npmjs.com/misc/scripts#path ).

@brettz9
Copy link
Contributor Author

brettz9 commented Jun 8, 2018

Btw, I didn't include the updates I get to the browser files--I guess you were either missing them from a prior update or your dependencies may have since updated to build the files differently.

And in that connection, you might also want to commit package-lock.json (as built by and recommended by current versions of npm) so that testers are using the same dependency versions.

@whitlockjc
Copy link
Owner

I do like the idea of having an .eslintignore file. Do you mind reverting your change to package.json and adding bower_components to the list of files in .eslintignore, and coverage? Actually, .gitignore is a good place to look just in case.

@brettz9
Copy link
Contributor Author

brettz9 commented Jun 8, 2018

I've added bower_components and coverage. Two other directories are .idea/ and vscode/ but I didn't know if they might contain JavaScript files?

As far as the package.json change, I removed the eslint script, but I added a simplification that works because of how npm adds binary files to the path. I can't see any reason not to add that...

@brettz9
Copy link
Contributor Author

brettz9 commented Jun 8, 2018

Also a gentle reminder--bower has been deprecated...

@whitlockjc
Copy link
Owner

I've not kept up with bower. What's its replacement?

@brettz9
Copy link
Contributor Author

brettz9 commented Jun 8, 2018

npm/yarn... See #117 (comment) . npm has made some improvements since yarn started to get traction back when bower recommended yarn e.g., https://blog.risingstack.com/yarn-vs-npm-node-js-package-managers/ .

@whitlockjc
Copy link
Owner

I'll give it a peek. :)

@brettz9
Copy link
Contributor Author

brettz9 commented Jun 9, 2018

Without needing to get into a decision about adding yarn support, I think at least going with including the files in npm would be a clear and simple option. People are using npm for a lot more than just Node these days.

Since one can sometimes forget to update both yarn.lock and package-lock.json (the file that gets created now by npm when doing a local npm install and which specifies the exact versions in use), I'd personally go with just the latter if not supporting both.

- npm: Simplify `test` script
@brettz9 brettz9 closed this Jan 9, 2019
@brettz9 brettz9 deleted the linting branch January 9, 2019 23:58
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