Update deps #11

bemailloux · 2019-11-15T21:09:14Z

Continuing what @jwong-d2l started in #10: ~~remove vinyl-fs~~ (EDIT: vinyl-fs is back. Too many consumers of this module are still using vinyl), ~~modernize~~ delete babel, and update dev dependencies. With these changes there are 0 known vulnerabilities in this package (down from 38)!

Testing:

Unit tests pass + coverage increased
I npm linked it to my local clone of bdp-visualizations-fra and ran the script. It builds appconfig.json correctly, in the location specified in dist. The file is always called appconfig.json, which I'm pretty sure was the case before.
yargs command line arguments and aliases still work
reading arguments from a package.json still works
chalk still prints in the correct colours

Note: if you're running from command line, frau-appconfig-builder will likely fail because you haven't specified an appClass.

bemailloux · 2019-11-15T21:15:00Z

Argh. Looks like I will need to update node as part of this change

coveralls · 2019-11-15T21:20:26Z

Coverage increased (+6.4%) to 100.0% when pulling 4504df1 on bemailloux:update-deps into e1dc706 on Brightspace:master.

bemailloux · 2019-11-17T17:17:25Z

Not sure if we also want to update the other dependencies that are out of date (yargs, chalk, semver)

dbatiste · 2019-11-18T13:56:52Z

README.md

@@ -102,7 +102,7 @@ This repository is configured with [EditorConfig](http://editorconfig.org) rules
 [npm-url]: https://www.npmjs.org/package/frau-appconfig-builder
 [npm-image]: https://img.shields.io/npm/v/frau-appconfig-builder.svg
 [ci-url]: https://travis-ci.org/Brightspace/frau-appconfig-builder
-[ci-image]: https://img.shields.io/travis-ci/Brightspace/frau-appconfig-builder.svg


I think we want this URL: https://img.shields.io/travis/com/Brightspace/frau-appconfig-builder.svg

We try to use the badges from img.shields.io for consistency, otherwise they are different size/font and they look terrible.

Ok thanks! The existing badge was broken but the one in the link you provided works

Cool yeah, I think we probably have a few that are broken due to the switch to travis.com.

dbatiste

This looks ok to me. I'd probably try to update the other dependencies you asked about as well. It'd be good to get someone elses eyes on this since I haven't worked with appconfig in years.

bemailloux · 2019-11-18T14:06:07Z

TODO:

Update other dependencies
Update travis badge

jwong-d2l · 2019-11-18T14:53:08Z

bin/appconfigbuildercli


-argv = argv.usage('Usage: frau-appconfig-builder [--appfile|-f] [--dist|-d] [--loader|l] [--showloading|-s]')
+argv = argv.usage('Usage: frau-appconfig-builder [--appfile|-f] [--dist|-d] [--loader|-l] [--showloading|-s]')


jwong-d2l · 2019-11-18T14:53:20Z

.travis.yml

@@ -1,6 +1,6 @@
 language: node_js
 node_js:
-  - '0.12'
+  - '6.9'


wow, really? It was on 0.12?

Lol yeah. One of the dependencies used const and travis freaked out when I tried to test it

omsmith

Have you searched if people were consuming this programmatically, or are they just using the cli?

Transformations done seem okay to me, though they are breaking (hence the question above). The end result is still imperfect error-handling wise, but not something that needs to be fixed here.

omsmith · 2019-11-18T15:28:47Z

test/mocha.opts

@@ -1 +1 @@
--compilers js:babel/register -R spec
+--require @babel/register -R spec


I don't believe babel is needed at this point.

That's because recent versions of node support ES5/ES6 features natively, right?

Right. I expect this was mostly in here to be able to use, say, const in the tests.

omsmith · 2019-11-18T15:31:08Z

.travis.yml

@@ -1,6 +1,6 @@
 language: node_js
 node_js:
-  - '0.12'
+  - '6.9'


Node.js 6 was EOL in April. If we're updating things, not a ton of point in including or, or at least having it as the only version.

Maybe do a list of 8, 10 and 12

bemailloux · 2019-11-18T15:59:02Z

Have you searched if people were consuming this programmatically

Looks like they are, which is not a case I considered. Example: https://github.com/Brightspace/simplified-interface-checklist/blob/master/bin/build-appconfig.js

If I still want to go ahead with this change, I guess I should bump the major version number (1.0.0 -> 2.0.0)?

omsmith · 2019-11-18T16:05:29Z

If I still want to go ahead with this change, I guess I should bump the major version number (1.0.0 -> 2.0.0)?

Yeah, and ideally open a PR against the users so the company isn't sitting stale. I know that makes more work for you though.

If you wanted to stay non-breaking, you'd want to put vinyl back in probably.

jwong-d2l · 2019-11-18T20:19:03Z

test/htmlAppConfigBuilder .js

@@ -1,8 +1,8 @@
-import chai from 'chai';


this wasn't you.. but anyone else notice that space in the filename?

bemailloux · 2019-11-18T21:32:59Z

you'd want to put vinyl back in probably.

vinyl-fs@3.0.3 doesn't have any vulnerabilities, so I did that. I also put this package's version number back to 1.1.0.

I know that makes more work for you though.

For something that was part of inspiration I don't want to spend that much time on this :)

bemailloux added 4 commits November 15, 2019 16:00

Update dev deps: mocha coveralls istanbul-harmony->nyc chai sinon

824854c

Modernize babel

57a51d1

Update frau-publisher

b506992

Remove vinyl-fs and streamifier - use node built-ins instead

813e03b

Update to node 6

e8afaf4

bemailloux added 3 commits November 15, 2019 16:22

Version bump

2fe6547

MOAR tests

1cab9a4

Stop console logging extra stuff

e3d2d1b

dbatiste reviewed Nov 18, 2019

View reviewed changes

dbatiste requested a review from omsmith November 18, 2019 14:00

dbatiste approved these changes Nov 18, 2019

View reviewed changes

Update additional deps + change travis badge

6f901cf

jwong-d2l reviewed Nov 18, 2019

View reviewed changes

omsmith requested changes Nov 18, 2019

View reviewed changes

Bump major version, de-babelify, test on node 8/10/12 on travis

e9043ae

jwong-d2l reviewed Nov 18, 2019

View reviewed changes

jwong-d2l approved these changes Nov 18, 2019

View reviewed changes

Back to vinyl-fs to not break programmatic users of appconfig builder

4504df1

omsmith approved these changes Nov 21, 2019

View reviewed changes

bemailloux merged commit 9e08e22 into Brightspace:master Nov 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update deps #11

Update deps #11

bemailloux commented Nov 15, 2019 •

edited

bemailloux commented Nov 15, 2019

coveralls commented Nov 15, 2019 •

edited

bemailloux commented Nov 17, 2019

dbatiste Nov 18, 2019

bemailloux Nov 18, 2019

dbatiste Nov 18, 2019

dbatiste left a comment

bemailloux commented Nov 18, 2019 •

edited

jwong-d2l Nov 18, 2019

jwong-d2l Nov 18, 2019

bemailloux Nov 18, 2019

omsmith left a comment

omsmith Nov 18, 2019

bemailloux Nov 18, 2019

omsmith Nov 18, 2019

omsmith Nov 18, 2019

bemailloux commented Nov 18, 2019

omsmith commented Nov 18, 2019

jwong-d2l Nov 18, 2019

bemailloux commented Nov 18, 2019 •

edited


		argv = argv.usage('Usage: frau-appconfig-builder [--appfile\|-f] [--dist\|-d] [--loader\|l] [--showloading\|-s]')
		argv = argv.usage('Usage: frau-appconfig-builder [--appfile\|-f] [--dist\|-d] [--loader\|-l] [--showloading\|-s]')

		@@ -1 +1 @@
		--compilers js:babel/register -R spec
		--require @babel/register -R spec

Update deps #11

Update deps #11

Conversation

bemailloux commented Nov 15, 2019 • edited

bemailloux commented Nov 15, 2019

coveralls commented Nov 15, 2019 • edited

bemailloux commented Nov 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbatiste left a comment

Choose a reason for hiding this comment

bemailloux commented Nov 18, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bemailloux commented Nov 18, 2019

omsmith commented Nov 18, 2019

Choose a reason for hiding this comment

bemailloux commented Nov 18, 2019 • edited

bemailloux commented Nov 15, 2019 •

edited

coveralls commented Nov 15, 2019 •

edited

bemailloux commented Nov 18, 2019 •

edited

bemailloux commented Nov 18, 2019 •

edited