build: move inline resources script to tools #4894

devversion · 2017-05-31T16:22:34Z

Moves the inline resources script to the tools/gulp/packaging folder. This is necessary when exposing the build tools (e.g for flex-layout)
This also makes the packaging more clean because no external script is referenced (also switched to TypeScript and improved code)

* Moves the inline resources script to the `tools/gulp/packaging` folder. This is necessary when exposing the build tools (e.g for flex-layout) * This also makes the packaging more clean because no external script is referenced (also switched to TypeScript and improved code)

jelbourn

Want to do a bit of refactoring while already touching this file?

jelbourn · 2017-05-31T16:26:02Z

tools/gulp/packaging/inline-resources.ts

+import {sync as glob} from 'glob';
+
+/** Finds all JavaScript files and inlines all external resources of Angular components. */
+export function inlineResourcesFolder(folderPath: string) {


how about just inlineAllResources?

I thought about that but AllResources sounds like it inlines all resources a given file.

Then maybe inlineAllResourceForDirectory (and then inlineResourcesForFile)?

Yeah inlineAllResourceForDirectory sounds good

Actually why not just inlineResourcesForDirectory?

jelbourn · 2017-05-31T16:29:38Z

tools/gulp/packaging/inline-resources.ts

+}
+
+/** Inlines the templates of Angular components for a specified source file. */
+function inlineTemplate(fileContent: string, filePath: string) {


Are the inlined resources here minified?

No, the minifying happens before by Gulp.

jelbourn · 2017-05-31T16:34:40Z

tools/gulp/packaging/inline-resources.ts

+/** Inlines the external styles of Angular components for a specified source file. */
+function inlineStyles(fileContent: string, filePath: string) {
+  return fileContent.replace(/styleUrls:\s*(\[[\s\S]*?])/gm, (match, styleUrls) => {
+    // The RegExp matches the array of external style files. This is a string right now and


Expand comment to include something like

// styleUrls will be a string that looks something like "['button.css', 'other.css']"

jelbourn · 2017-05-31T16:37:56Z

tools/gulp/packaging/inline-resources.ts

+
+/** Inlines the external styles of Angular components for a specified source file. */
+function inlineStyles(fileContent: string, filePath: string) {
+  return fileContent.replace(/styleUrls:\s*(\[[\s\S]*?])/gm, (match, styleUrls) => {


Maybe rename styleUrls to styleUrlsValue to make it more clear that it's just yet the real array of styleUrl strings. Then prsedUrls below could just change to styleUrls.

Yeah I like that.

jelbourn · 2017-05-31T16:42:08Z

tools/gulp/packaging/inline-resources.ts

+      const stylePath = join(dirname(filePath), styleUrl);
+      const styleContent = loadResourceFile(stylePath);
+      return `"${styleContent}"`;
+    }).join(',\n') + ']';


Maybe refactor this to

const styles = parsedUrls .map(url => join(dirname(filePath), url)) .map(path => loadResourceFile(path)); return `styles: [${styleContents.join(',')}]`;

(the newline shouldn't be necessary AFAICT)

I like that. Not sure about the line break. I didn't write it but I will test it.

jelbourn · 2017-05-31T16:42:46Z

tools/gulp/packaging/inline-resources.ts

+function inlineTemplate(fileContent: string, filePath: string) {
+  return fileContent.replace(/templateUrl:\s*'([^']+?\.html)'/g, (match, templateUrl) => {
+    const templateFile = join(dirname(filePath), templateUrl);
+    const templateContent = loadResourceFile(templateFile);


Rename variables to templatePath and template?

templatePath sounds good. For the template I'd like to keep the content to indicate it's the text.

That's fine too

devversion · 2017-05-31T18:05:39Z

@jelbourn Addressed your comments. The inlineStyles method now looks way better. Thanks

jelbourn

LGTM, just one tiny comment; can add merge-ready when done

jelbourn · 2017-05-31T18:29:29Z

tools/gulp/packaging/inline-resources.ts

+function inlineStyles(fileContent: string, filePath: string) {
+  return fileContent.replace(/styleUrls:\s*(\[[\s\S]*?])/gm, (match, styleUrlsValue) => {
+    // The RegExp matches the array of external style files. This is a string right now and
+    // can to be parsed using the `eval` method. The value looks like "['AAA.css', 'BBB.css"]"


That second-to-last " should be a '

angular-automatic-lock-bot · 2019-09-07T16:03:20Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

devversion added pr: merge safe labels May 31, 2017

devversion assigned jelbourn May 31, 2017

devversion requested a review from jelbourn May 31, 2017 16:22

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 31, 2017

jelbourn reviewed May 31, 2017

View reviewed changes

Address comments

3d7a8a3

devversion force-pushed the build/move-inline-resources-script branch from 10e7552 to 3d7a8a3 Compare May 31, 2017 18:05

jelbourn approved these changes May 31, 2017

View reviewed changes

jelbourn added pr: lgtm and removed pr: needs review labels May 31, 2017

Update comment

ef94b4b

devversion added the action: merge The PR is ready for merge by the caretaker label May 31, 2017

mmalerba merged commit 52b6692 into angular:master May 31, 2017

willshowell mentioned this pull request Jun 1, 2017

sidenav open/close missing animation #4910

Closed

devversion deleted the build/move-inline-resources-script branch November 11, 2017 10:23

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019

build: move inline resources script to tools #4894

build: move inline resources script to tools #4894

Uh oh!

Conversation

devversion commented May 31, 2017

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devversion May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devversion May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devversion commented May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Sep 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

devversion May 31, 2017 •

edited

Loading

jelbourn May 31, 2017 •

edited

Loading

devversion May 31, 2017 •

edited

Loading

devversion commented May 31, 2017 •

edited

Loading