Skip to content

Commit 0f8ccd9

Browse files
izaeraizaera
committed
Add more missing feedback from review
Fixes #185
1 parent 597981b commit 0f8ccd9

File tree

8 files changed

+47
-42
lines changed

8 files changed

+47
-42
lines changed

‎bin/build-demo.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,6 @@ globby.sync('src/demo/modules/**/*.js').forEach(file => {
2929

3030
copy(
3131
path.join('src', 'demo', 'modules', filePath),
32-
path.join('build', 'demo', 'modules', 'filePath}')
32+
path.join('build', 'demo', 'modules', filePath)
3333
);
3434
});

‎bin/demo-server.js‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,27 @@
1-
const http = require('http');
21
const fs = require('fs');
2+
const http = require('http');
3+
const path = require('path');
34
const url = require('url');
45

56
const processResource = (resource, res) => {
6-
fs.createReadStream('build/demo' + resource).pipe(res);
7+
fs.createReadStream(path.join('build', 'demo', resource)).pipe(res);
78
};
89

910
const processModules = (resource, res) => {
10-
let params = url.parse(resource, true);
11+
const params = url.parse(resource, true);
1112

1213
let modules = params.query.modules;
1314

14-
modules = modules.replace(/,/g, '_');
15-
modules = modules.replace(/\//g, '_');
15+
modules = modules.replace(/[,/]/g, '_');
1616

17-
let path = 'build/demo/resolutions/' + modules + '.json';
17+
const filePath = path.join(
18+
'build',
19+
'demo',
20+
'resolutions',
21+
modules + '.json'
22+
);
1823

19-
fs.createReadStream(path).pipe(res);
24+
fs.createReadStream(filePath).pipe(res);
2025
};
2126

2227
const server = http.createServer((req, res) => {
@@ -28,7 +33,7 @@ const server = http.createServer((req, res) => {
2833

2934
if (resource.startsWith('/o/js_resolve_modules')) {
3035
processModules(resource, res);
31-
} else if (fs.existsSync('build/demo' + resource)) {
36+
} else if (fs.existsSync(path.join('build', 'demo', resource))) {
3237
processResource(resource, res);
3338
} else {
3439
res.end();

‎src/loader/__tests__/config.js‎

Lines changed: 20 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,11 @@ describe('Config', () => {
77
config = new Config();
88
});
99

10-
it('should create an instance of Config without existing data', () => {
10+
it('creates an instance of Config without existing data', () => {
1111
expect(config.getModules()).toHaveLength(0);
1212
});
1313

14-
it('should add new module', () => {
14+
it('adds new module', () => {
1515
const addedModule = config.addModule('a-module');
1616
const modules = config.getModules();
1717

@@ -20,14 +20,14 @@ describe('Config', () => {
2020
expect(modules[0].name).toBe('a-module');
2121
});
2222

23-
it('should return existing module when addModule called twice', () => {
23+
it('returns existing module when addModule called twice', () => {
2424
const addedModule1 = config.addModule('a-module');
2525
const addedModule2 = config.addModule('a-module');
2626

2727
expect(addedModule1).toBe(addedModule2);
2828
});
2929

30-
it('should map a module to its alias', () => {
30+
it('maps a module to its alias', () => {
3131
const addedModule = config.addModule('liferay@1.0.0');
3232

3333
config.addMappings({liferay: 'liferay@1.0.0'});
@@ -37,7 +37,7 @@ describe('Config', () => {
3737
expect(mappedModule).toBe(addedModule);
3838
});
3939

40-
it('should respect "exactMatch" mappings', () => {
40+
it('respects "exactMatch" mappings', () => {
4141
const addedModule = config.addModule('liferay@1.0.0/index');
4242

4343
config.addMappings({
@@ -56,7 +56,7 @@ describe('Config', () => {
5656
expect(mappedModule).toBe(addedModule);
5757
});
5858

59-
it('should map an array of modules to their aliases', () => {
59+
it('maps an array of modules to their aliases', () => {
6060
const addedModule1 = config.addModule('liferay@1.0.0');
6161
const addedModule2 = config.addModule('liferay@2.0.0');
6262

@@ -70,7 +70,7 @@ describe('Config', () => {
7070
expect(mappedModules).toEqual([addedModule1, addedModule2]);
7171
});
7272

73-
it('should map a module via a mapping function', () => {
73+
it('maps a module via a mapping function', () => {
7474
const addedModule1 = config.addModule('liferaytest');
7575
const addedModule2 = config.addModule('liferay2test');
7676

@@ -83,25 +83,21 @@ describe('Config', () => {
8383
expect(mappedModules).toEqual([addedModule1, addedModule2]);
8484
});
8585

86-
it(
87-
'should ignore a mapping function if a more specific module mapping ' +
88-
'exists',
89-
() => {
90-
const addedModule1 = config.addModule('liferay@1.0.0');
91-
const addedModule2 = config.addModule('liferay2test');
86+
it('ignores a mapping function if a more specific module mapping exists', () => {
87+
const addedModule1 = config.addModule('liferay@1.0.0');
88+
const addedModule2 = config.addModule('liferay2test');
9289

93-
config.addMappings({
94-
'liferay': 'liferay@1.0.0',
95-
'*': name => name + 'test',
96-
});
90+
config.addMappings({
91+
'liferay': 'liferay@1.0.0',
92+
'*': name => name + 'test',
93+
});
9794

98-
const mappedModules = config.getModules(['liferay', 'liferay2']);
95+
const mappedModules = config.getModules(['liferay', 'liferay2']);
9996

100-
expect(mappedModules).toEqual([addedModule1, addedModule2]);
101-
}
102-
);
97+
expect(mappedModules).toEqual([addedModule1, addedModule2]);
98+
});
10399

104-
it('should apply exactMatches first', () => {
100+
it('applies exactMatches first', () => {
105101
const addedModule1 = config.addModule('liferay@1.0.0/index');
106102
const addedModule2 = config.addModule('liferay@2.0.0/main');
107103
const addedModule3 = config.addModule('liferay@2.0.0');
@@ -132,7 +128,7 @@ describe('Config', () => {
132128
expect(mappedModule).toBe(addedModule4);
133129
});
134130

135-
it('should stop replacement for exact identity matches', () => {
131+
it('stops replacement for exact identity matches', () => {
136132
const addedModule = config.addModule('liferay/index');
137133

138134
config.addMappings({
@@ -145,7 +141,7 @@ describe('Config', () => {
145141
expect(mappedModule).toBe(addedModule);
146142
});
147143

148-
it('should map local modules correctly', () => {
144+
it('maps local modules correctly', () => {
149145
const currentModule = config.addModule('isobject@2.1.0/index');
150146
const dependency1 = config.addModule('isarray@1.0.0');
151147
const dependency2 = config.addModule('isarray@1.0.0/index');

‎src/loader/__tests__/dependency-resolver.js‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,14 @@ import DependencyResolver from '../dependency-resolver';
44
describe('DependencyResolver', () => {
55
let config;
66
let dependencyResolver;
7+
let originalFetch;
78

89
beforeEach(() => {
910
config = new Config();
1011
dependencyResolver = new DependencyResolver(config);
1112

12-
window.fetch = jest.fn().mockImplementation(param => {
13+
originalFetch = window.fetch;
14+
window.fetch = jest.fn(param => {
1315
const encodedModules = param.replace(
1416
`${config.resolvePath}?modules=`,
1517
''
@@ -23,6 +25,10 @@ describe('DependencyResolver', () => {
2325
});
2426
});
2527

28+
afterEach(() => {
29+
window.fetch = originalFetch;
30+
});
31+
2632
it('should throw an exception if no modules are specified', () => {
2733
expect(() => dependencyResolver.resolve()).toThrow();
2834
});

‎src/loader/__tests__/resolvable-promise.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ describe('ResolvablePromise', () => {
3939
}, 100);
4040
});
4141

42-
it('reject should invoke pre and post registered catchs', done => {
42+
it('reject should invoke pre and post registered catches', done => {
4343
let preCalled;
4444
let postCalled;
4545

‎src/loader/config.js‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ export default class Config {
2525
}
2626

2727
/**
28-
* Whether or not to explain how require() calls are resolved
28+
* Whether to explain how require() calls are resolved
2929
*/
3030
get explainResolutions() {
3131
return this._config.explainResolutions;
@@ -46,14 +46,14 @@ export default class Config {
4646
}
4747

4848
/**
49-
* The path invoke when resolving module dependencies
49+
* The path to use when calling the server to resolve module dependencies
5050
*/
5151
get resolvePath() {
5252
return this._config.resolvePath;
5353
}
5454

5555
/**
56-
* Whether or not to combine module requests into combo URLs
56+
* Whether to combine module requests into combo URLs
5757
*/
5858
get combine() {
5959
return this._config.combine;
@@ -260,7 +260,6 @@ export default class Config {
260260
*/
261261
_mapExactMatch(module, maps) {
262262
for (let alias in maps) {
263-
/* istanbul ignore else */
264263
if (Object.prototype.hasOwnProperty.call(maps, alias)) {
265264
const aliasValue = maps[alias];
266265

@@ -283,7 +282,6 @@ export default class Config {
283282
*/
284283
_mapPartialMatch(module, maps) {
285284
for (let alias in maps) {
286-
/* istanbul ignore else */
287285
if (Object.prototype.hasOwnProperty.call(maps, alias)) {
288286
let aliasValue = maps[alias];
289287

‎src/loader/dependency-resolver.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ export default class DependencyResolver {
2525
*/
2626
resolve(modules) {
2727
if (modules === undefined || modules.length == 0) {
28-
throw new Error(`Invalid argument 'modules': ${modules}`);
28+
throw new Error(`Argument 'modules' cannot be undefined or empty`);
2929
}
3030

3131
const config = this._config;

‎src/loader/module.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ export default class Module {
4949
}
5050

5151
/**
52-
* Get the fetch promise which if fulfilled when the script containing the
52+
* Get the fetch promise which is fulfilled when the script containing the
5353
* module definition has been loaded/failed.
5454
*
5555
* Note that a module may be defined even if it is not yet fetched because

0 commit comments

Comments
 (0)