Skip to content

Fix stuff #11200

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jun 13, 2018
Merged

Fix stuff #11200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dd3ff94
refactor(@angular/pwa): minor refactorings to make code cleaner
gkalpak Jun 1, 2018
9d92390
refactor(@schematics/angular): minor refactorings
gkalpak Jun 11, 2018
44d947a
fix(@schematics/angular): allow ServiceWorker to work with `baseHref`
gkalpak Jun 1, 2018
ef14464
style(@schematics/angular): minor improvements in `ngsw-config.json`
gkalpak Jun 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 31 additions & 50 deletions packages/angular/pwa/pwa/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import {
SchematicsException,
Tree,
apply,
branchAndMerge,
chain,
externalSchematic,
mergeWith,
Expand All @@ -39,15 +38,14 @@ function addServiceWorker(options: PwaOptions): Rule {

function getIndent(text: string): string {
let indent = '';
let hitNonSpace = false;
text.split('')
.forEach(char => {
if (char === ' ' && !hitNonSpace) {
indent += ' ';
} else {
hitNonSpace = true;
}
}, 0);

for (const char of text) {
if (char === ' ' || char === '\t') {
indent += char;
} else {
break;
}
}

return indent;
}
Expand All @@ -70,43 +68,32 @@ function updateIndexFile(options: PwaOptions): Rule {
const content = buffer.toString();
const lines = content.split('\n');
let closingHeadTagLineIndex = -1;
let closingHeadTagLine = '';
let closingBodyTagLineIndex = -1;
let closingBodyTagLine = '';
lines.forEach((line: string, index: number) => {
if (/<\/head>/.test(line) && closingHeadTagLineIndex === -1) {
closingHeadTagLine = line;
lines.forEach((line, index) => {
if (closingHeadTagLineIndex === -1 && /<\/head>/.test(line)) {
closingHeadTagLineIndex = index;
}

if (/<\/body>/.test(line) && closingBodyTagLineIndex === -1) {
closingBodyTagLine = line;
} else if (closingBodyTagLineIndex === -1 && /<\/body>/.test(line)) {
closingBodyTagLineIndex = index;
}
});

const headTagIndent = getIndent(closingHeadTagLine) + ' ';
const headIndent = getIndent(lines[closingHeadTagLineIndex]) + ' ';
const itemsToAddToHead = [
'<link rel="manifest" href="manifest.json">',
'<meta name="theme-color" content="#1976d2">',
];

const textToInsertIntoHead = itemsToAddToHead
.map(text => headTagIndent + text)
.join('\n');

const bodyTagIndent = getIndent(closingBodyTagLine) + ' ';
const itemsToAddToBody
= '<noscript>Please enable JavaScript to continue using this application.</noscript>';

const textToInsertIntoBody = bodyTagIndent + itemsToAddToBody;
const bodyIndent = getIndent(lines[closingBodyTagLineIndex]) + ' ';
const itemsToAddToBody = [
'<noscript>Please enable JavaScript to continue using this application.</noscript>',
Copy link
Member

@clydin clydin Jun 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Adding this to the actual template index.html may make sense since it applies to a standard web applications as well. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

True. I guess this was added as poart of @angular/pwa, because good PWA practices include having fallback content for when JS is disabled, but this is indeed useful for non-PWA apps as well.
Should Imove it to index.html?

Copy link
Member

Choose a reason for hiding this comment

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

I think it eventually should but probably should be left to a separate PR. However, the addition here would need to stay either way to support projects that didn't get created with the new index.html. The code here should be augment now though to check if the noscriptelement is already present and only add if not. Since someone could have added the element manually as well.

Copy link
Member Author

@gkalpak gkalpak Jun 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, I just realized that a proper solution should also check whether link[rel="manifest"] and meta[name="theme-color"] already exist as well. Since this PR is only refactoring that part of the code (no behavior changes), I'd rather leave the checks for another PR.

Copy link
Member

Choose a reason for hiding this comment

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

Good point and works for me.

];

const updatedIndex = [
...lines.slice(0, closingHeadTagLineIndex),
textToInsertIntoHead,
...itemsToAddToHead.map(line => headIndent + line),
...lines.slice(closingHeadTagLineIndex, closingBodyTagLineIndex),
textToInsertIntoBody,
...lines.slice(closingBodyTagLineIndex),
...itemsToAddToBody.map(line => bodyIndent + line),
...lines.slice(closingHeadTagLineIndex),
].join('\n');

host.overwrite(path, updatedIndex);
Expand Down Expand Up @@ -137,12 +124,9 @@ function addManifestToAssetsConfig(options: PwaOptions) {
['build', 'test'].forEach((target) => {

const applyTo = architect[target].options;
const assets = applyTo.assets || (applyTo.assets = []);

if (!applyTo.assets) {
applyTo.assets = [assetEntry];
} else {
applyTo.assets.push(assetEntry);
}
assets.push(assetEntry);

});

Expand All @@ -163,27 +147,24 @@ export default function (options: PwaOptions): Rule {
throw new SchematicsException(`PWA requires a project type of "application".`);
}

const assetPath = join(project.root as Path, 'src', 'assets');
const sourcePath = join(project.root as Path, 'src');
const assetsPath = join(sourcePath, 'assets');

options.title = options.title || options.project;

const templateSource = apply(url('./files/assets'), [
template({
...options,
}),
move(assetPath),
const rootTemplateSource = apply(url('./files/root'), [
template({ ...options }),
move(sourcePath),
]);
const assetsTemplateSource = apply(url('./files/assets'), [
template({ ...options }),
move(assetsPath),
]);

return chain([
addServiceWorker(options),
branchAndMerge(chain([
mergeWith(templateSource),
])),
mergeWith(apply(url('./files/root'), [
template({...options}),
move(sourcePath),
])),
mergeWith(rootTemplateSource),
mergeWith(assetsTemplateSource),
updateIndexFile(options),
addManifestToAssetsConfig(options),
]);
Expand Down
44 changes: 23 additions & 21 deletions packages/schematics/angular/service-worker/files/ngsw-config.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,26 @@
{
"index": "/index.html",
"assetGroups": [{
"name": "app",
"installMode": "prefetch",
"resources": {
"files": [
"/favicon.ico",
"/index.html",
"/*.css",
"/*.js"
]
"assetGroups": [
{
"name": "app",
"installMode": "prefetch",
"resources": {
"files": [
"/favicon.ico",
"/index.html",
"/*.css",
"/*.js"
]
}
}, {
"name": "assets",
"installMode": "lazy",
"updateMode": "prefetch",
"resources": {
"files": [
"/assets/**"
]
}
}
}, {
"name": "assets",
"installMode": "lazy",
"updateMode": "prefetch",
"resources": {
"files": [
"/assets/**"
]
}
}]
}
]
}
6 changes: 3 additions & 3 deletions packages/schematics/angular/service-worker/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,11 @@ function addDependencies(): Rule {
if (coreDep === null) {
throw new SchematicsException('Could not find version.');
}
const platformServerDep = {
const serviceWorkerDep = {
...coreDep,
name: packageName,
};
addPackageJsonDependency(host, platformServerDep);
addPackageJsonDependency(host, serviceWorkerDep);

return host;
};
Expand Down Expand Up @@ -131,7 +131,7 @@ function updateAppModule(options: ServiceWorkerOptions): Rule {

// register SW in app module
const importText =
`ServiceWorkerModule.register('/ngsw-worker.js', { enabled: environment.production })`;
`ServiceWorkerModule.register('ngsw-worker.js', { enabled: environment.production })`;
moduleSource = getTsSourceFile(host, modulePath);
const metadataChanges = addSymbolToNgModuleMetadata(
moduleSource, modulePath, 'imports', importText);
Expand Down
4 changes: 2 additions & 2 deletions packages/schematics/angular/service-worker/index_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,8 @@ describe('Service Worker Schematic', () => {
const tree = schematicRunner.runSchematic('service-worker', defaultOptions, appTree);
const pkgText = tree.readContent('/projects/bar/src/app/app.module.ts');
// tslint:disable-next-line:max-line-length
const regex = /ServiceWorkerModule\.register\('\/ngsw-worker.js\', { enabled: environment.production }\)/;
expect(pkgText).toMatch(regex);
const expectedText = 'ServiceWorkerModule.register(\'ngsw-worker.js\', { enabled: environment.production })';
expect(pkgText).toContain(expectedText);
});

it('should put the ngsw-config.json file in the project root', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/schematics/angular/utility/dependencies.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export function addPackageJsonDependency(tree: Tree, dependency: NodeDependency)
// Haven't found the dependencies key, add it to the root of the package.json.
appendPropertyInAstObject(recorder, packageJsonAst, dependency.type, {
[dependency.name]: dependency.version,
}, 4);
}, 2);
} else if (depsNode.kind === 'object') {
// check if package already added
const depNode = findPropertyInAstObject(depsNode, dependency.name);
Expand Down
2 changes: 1 addition & 1 deletion packages/schematics/angular/utility/json-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ export function insertPropertyInAstObjectInOrder(

recorder.insertRight(
insertIndex,
`${indentStr}`
indentStr
+ `"${propertyName}": ${JSON.stringify(value, null, 2).replace(/\n/g, indentStr)}`
+ ',',
);
Expand Down