Refactoring #11

denar90 · 2017-01-17T23:32:55Z

coffee -> es6 + refactoring

shvaikalesh · 2017-01-18T02:27:46Z

Let's not commit yarn.lock. Useless trash.

shvaikalesh · 2017-01-18T04:51:35Z

index.js

+    }
+
+    // Defaults options
+    this.options = {


const defaultOptions = and remove the comment

shvaikalesh · 2017-01-18T04:51:48Z

index.js

+
+    // Defaults options
+    this.options = {
+      ignore: /[\\/][.]/,


shvaikalesh · 2017-01-18T04:52:01Z

index.js

+      manifestFile: 'appcache.appcache'
+    };
+
+    // Merge config


remove the comment pls

shvaikalesh · 2017-01-18T04:52:49Z

index.js

+}
+
+AppCacheCompiler.prototype.brunchPlugin = true;
+AppCacheCompiler.prototype.staticTargetExtension = 'html';


This one is extra

denysdovhan · 2017-02-18T12:15:30Z

README.md

-      staticRoot: '/static'
-      network: ['*']
+```js
+exports.config = {


module.exports = {

denysdovhan · 2017-02-18T12:40:26Z

package.json

+    "mocha": "^3",
+    "sinon": "^1.17.7",
+    "eslint": "^3.12.2",
+    "eslint-config-brunch": "brunch/eslint-config-brunch"


^1, ^3 etc.

shvaikalesh · 2017-02-18T18:37:45Z

index.js

+  }
+
+  processFile(file) {
+    if (!/[.]appcache$/.test(file.path) && !this.config.ignore.test(file.path) && !this.paths.includes(file.path)) {


path.endsWith('.appcache')

shvaikalesh · 2017-02-18T18:38:10Z

index.js

+    return (Array.from(Object.keys(obj).sort()).map(k => `${k} ${obj[k]}`)).join('\n');
+  };
+
+  _readStream(path) {


The whole point of rewrite is to not have this method. Use file.data.

shvaikalesh · 2017-02-18T18:38:47Z

package.json

@@ -8,14 +8,18 @@
    "type": "git",
    "url": "[email protected]:brunch/appcache-brunch.git"
  },
-  "main": "./lib/index",
+  "main": "index.js",


It is default value. We can remove it.

Isn't quite obvious, though

On pair with index.html.

shvaikalesh · 2017-02-18T18:47:11Z

index.js

+  }
+
+  _format(obj) {
+    return (Array.from(Object.keys(obj).sort()).map(k => `${k} ${obj[k]}`)).join('\n');


- Array.from(iter).map(fn) + Array.from(iter, fn)

shvaikalesh · 2017-02-18T18:48:41Z

index.js

+    try {
+      if (this.changedFileContentShasum) {
+        this.pathsToCache = files
+          .filter(file => !/[.]appcache$/.test(file.path) && !this.config.ignore.test(file.path) && !this.pathsToCache.includes(file.path))


path.endsWith('.appcache') and consider extracting this fn.

denysdovhan · 2017-02-19T23:15:50Z

index.js

          .map(file => file.path)
          .sort();

+        this.pathsToCache.push(...pathsToCache);


Personally, do not prefer this, but it looks acceptable.

denysdovhan · 2017-02-19T23:18:43Z

index.js

  }

+  _format(obj) {
+    return (Array.from(Object.keys(obj).sort(), k => `${k} ${obj[k]}`)).join('\n');


Extra () aren't necessary

Didn't we agree this should not be method?

Uses this => method
Otherwise => helper

Easy.

denysdovhan · 2017-02-19T23:22:45Z

test/test.js

+    });
+
+    it('should call "_processFile" method', () => {
+      return plugin.compile(mocks.files[0], () => {


Isn't plugin.compile returns a Promise? Why do you pass a callback then?

shvaikalesh · 2017-02-19T23:31:16Z

test/test.js

+    });
+
+    it('sets changedFileContentShasum', () => {
+      const file = mocks.files[0];


const [file] = mocks.files as we already dropped Node.js < 6.

shvaikalesh · 2017-02-19T23:31:38Z

test/test.js

+const mocks = require('./fixtures/mocks');
+const config = require('./fixtures/brunch.conf');
+
+const Plugin = require('../index');


require('..')

shvaikalesh · 2017-02-19T23:32:03Z

test/test.js

+    it('ignores appcache files', () => {
+      const file = mocks.ignoredFiles[0];
+      plugin._processFile(file);
+      assert.deepEqual(plugin.paths, []);


assert.empty

But assert doesn't have empty method...

It does in master.

In nodejs master?
Should we keep it as it is until new version be released?

shvaikalesh · 2017-02-19T23:32:32Z

test/test.js

+
+          assert.equal(actualAppCacheContent, expectedAppCacheContent);
+        })
+        .catch(e => console.log(e));


Let's remove this.

shvaikalesh · 2017-02-19T23:39:10Z

index.js

+${format(this.config.fallback)}
+
+CACHE:
+${(Array.from(this.pathsToCache).map(p => `${this.config.staticRoot}/${p}`)).join('\n')}


Array.from(this.pathsToCache, p => `${this.config.staticRoot}/${p}`)

and without extra ()

I disabled no-extra-parens in eslint config cause it gave false-positives. Maybe we should reconsider this.

I've fixed for format method but missed this one...

shvaikalesh · 2017-02-19T23:44:05Z

index.js

+      network: ['*'],
+      fallback: {},
+      staticRoot: '.',
+      manifestFile: 'appcache.appcache',


Array(2).fill('appcache').join('.')

shvaikalesh · 2017-02-19T23:44:59Z

index.js

+}
+
+AppCacheCompiler.prototype.brunchPlugin = true;
+AppCacheCompiler.prototype.pattern = /(?:)/;


const matchesAll = /(?:)/
AppCacheCompiler.prototype.pattern = matchesAll

denysdovhan · 2017-02-21T08:37:30Z

.travis.yml

+language: node_js
+  - osx
+node_js:
+  - 7


let's use just node — the latest version of Node.js

denar90 · 2017-05-25T21:01:11Z

I think it can go after real world testing aka brunch/brunch.github.io#267
thoughts?

denar90 · 2018-01-09T21:20:35Z

Should we stop working on a new version since all browsers support https://caniuse.com/#feat=serviceworkers service workers?

denar90 added 2 commits January 12, 2017 22:53

add yarn.lock

69d68c0

coffee -> es6 + refactoring

f1706b9

shvaikalesh reviewed Jan 18, 2017

View reviewed changes

index.js Outdated

}

// Defaults options

this.options = {

Copy link

shvaikalesh Jan 18, 2017

Choose a reason for hiding this comment

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

const defaultOptions = and remove the comment

shvaikalesh reviewed Jan 18, 2017

View reviewed changes

index.js Outdated

// Defaults options

this.options = {

ignore: /[\\/][.]/,

Copy link

shvaikalesh Jan 18, 2017

Choose a reason for hiding this comment

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

/\/\./

shvaikalesh reviewed Jan 18, 2017

View reviewed changes

index.js Outdated

manifestFile: 'appcache.appcache'

};

// Merge config

Copy link

shvaikalesh Jan 18, 2017

Choose a reason for hiding this comment

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

remove the comment pls

shvaikalesh reviewed Jan 18, 2017

View reviewed changes

denar90 added 8 commits January 18, 2017 13:18

cleanup

f85291f

get rid off yarn.lock

8dddf25

fixes for appcache file generating

10d77ca

add tests

dd5465c

bump mocha, chai versions

d2d1c05

add eslint

7f9e2f8

fix eslint errors

77d8ba1

update docs

343e72b

denar90 changed the title ~~Refactoring WIP~~ Refactoring Jan 18, 2017

improve stuff

85d58b1

denysdovhan reviewed Feb 18, 2017

View reviewed changes

shvaikalesh reviewed Feb 18, 2017

View reviewed changes

denar90 added 2 commits February 18, 2017 23:45

fixes after review

020659d

tests simplifications, minor changes

df11a6b

denysdovhan reviewed Feb 19, 2017

View reviewed changes

shvaikalesh reviewed Feb 19, 2017

View reviewed changes

denar90 added 3 commits February 20, 2017 23:51

minors

487f5fc

add travis

c724500

drop changelog

4919101

denysdovhan reviewed Feb 21, 2017

View reviewed changes

.travis.yml Outdated

language: node_js

- osx

node_js:

- 7

Copy link

Contributor

denysdovhan Feb 21, 2017

Choose a reason for hiding this comment

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

let's use just node — the latest version of Node.js

denar90 added 3 commits February 23, 2017 01:56

use latest node for travis

36c7771

use latest stable Node.js release for travis

3e605d3

fix fallback doc, add test

6da4411

Refactoring #11

Are you sure you want to change the base?

Refactoring #11

Conversation

denar90 commented Jan 17, 2017

shvaikalesh commented Jan 18, 2017

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

Choose a reason for hiding this comment

shvaikalesh Feb 18, 2017 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shvaikalesh Feb 19, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shvaikalesh Feb 19, 2017 • edited by denysdovhan Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denar90 commented May 25, 2017

denar90 commented Jan 9, 2018

shvaikalesh Feb 18, 2017 •

edited

Loading

shvaikalesh Feb 19, 2017 •

edited

Loading

shvaikalesh Feb 19, 2017 •

edited by denysdovhan

Loading