-
Notifications
You must be signed in to change notification settings - Fork 12k
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.
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
Fix stuff #11200
Conversation
packages/angular/pwa/pwa/index.ts
Outdated
|
||
return indent; | ||
// This RegExp is guaranteed to match any string (even if it is a zero-length match). | ||
return (/^ */.exec(text) as RegExpExecArray)[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about a quick loop instead of the regex (plus tab support):
let indent = '';
for (const char of text) {
if (char === ' ' || char === '\u0009') {
indent += char;
} else {
break;
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally like RegExps, but happy to change it to whatever you prefer (or even keep the original version).
I just happened to go through the files trying to fix something else and made some changes that made sense to me (and hopefully to others).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm partial to the more self explanatory nature of the loop above then the other two options. And it definitely performs better than the first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To each his own 😛
Happy to change it to a for
loop.
const textToInsertIntoBody = bodyTagIndent + itemsToAddToBody; | ||
const bodyIndent = getIndent(lines[closingBodyTagLineIndex]) + ' '; | ||
const itemsToAddToBody = [ | ||
'<noscript>Please enable JavaScript to continue using this application.</noscript>', |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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 noscript
element is already present and only add if not. Since someone could have added the element manually as well.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@gkalpak looks like there was a rebase error with the last commit. Its message is |
It was intentional (to make it easier to review the changes). It is a "fixup" commit. |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
The main fix is:
(Fixes ServiceWorker register generated by cli with base-href don't works #8515.)
This is a port of angular/devkit#999 from the archived
devkit
repo (minus the stuff already fixed in #11131).The NGSW parts have been approved by @alxhub here.