Replace ejs templates with a simple js file by realityking · Pull Request #397 · webpack/webpack-bundle-analyzer

realityking · 2020-11-24T00:56:56Z

This PR replaces the EJS based templates with a simple JS file using template strings.

webpack-bundle-analyzer uses very little HTML templating since most of the client is a SPA. What is used, can be replaced by a straightforward JS file. This drops 15(!) packages from the dependencies. I also like how all functions related to generating the HTML get pulled into the same file.

valscion · 2020-11-24T07:06:09Z

Please fix lint errors ☺️

th0r · 2020-11-24T09:36:18Z

+</html>`;
+}
+
+exports.renderViewer = renderViewer;


Let's move it above escapeJson function declaration please so exported members become clearly visible.

th0r · 2020-11-24T09:43:56Z

+}
+
+function renderViewer({title, enableWebSocket, chartData, defaultSizes, mode} = {}) {
+  return `<!DOCTYPE html>


I see a few drawbacks of this approach:

No syntax highlighting

Much more difficult to do conditional branching e.g. if/else if we'll need it in the future.

You have to remember to call _.escape manually

I'm not saying that I'm strongly against this PR, no. We can live with these drawbacks, but only if it brings some significant improvements. That's why I asked to provide difference in node_modules sizes.

Syntax highlighting could be added in some editors/plugins by adding a "fake" tag html. I'll add that as a commit so you can see what it looks like.

Agreed that conditionals are more cumbersome but do you expect the templates to get significantly more complex than they are today?

This is true (though you also have to be mindful of <%= vs <%- in EJS) and I wouldn't do it for a large server rendered app but as of today there's just two variables in the templates here that need to be escaped.

To me, this is trade-off of complexity. EJS is certainly nicer than template literals but for a template this small it's, IMHO, not worth the cost of the dependencies. Some of which are very old and don't even deduplicate.

th0r · 2020-11-24T09:48:47Z

This drops 15(!) packages from the dependencies

Could you also provide a difference between node_modules sizes after npm i --production?

th0r · 2020-11-24T10:15:07Z

@realityking may I ask you to not use force-push in the future please as it significantly complicates tracking of changes.

TrySound · 2020-11-24T10:57:18Z

ejs started to depend on jake build tool since v3 for some reason. v2 was dependency free.

realityking · 2020-11-24T10:58:35Z

Sorry about the force push, I'll refrain from them.

The size difference in node_modules is 1MB with this patch. (11 MB > 10 MB)

realityking · 2020-11-24T12:22:35Z

Just to further illustrate this point, these are the dependencies that would no longer be required:

├─┬ ejs@3.1.5
│ └─┬ jake@10.8.2
│   ├── async@0.9.2
│   ├─┬ chalk@2.4.2
│   │ ├─┬ ansi-styles@3.2.1
│   │ │ └─┬ color-convert@1.9.3
│   │ │   └── color-name@1.1.3
│   │ ├── escape-string-regexp@1.0.5
│   │ └─┬ supports-color@5.5.0
│   │   └── has-flag@3.0.0
│   ├─┬ filelist@1.0.1
│   │ └── minimatch@3.0.4 deduped
│   └─┬ minimatch@3.0.4
│     └─┬ brace-expansion@1.1.11
│       ├── balanced-match@1.0.0
│       └── concat-map@0.0.1

th0r · 2020-11-24T13:04:18Z

ejs started to depend on jake build

Hmm, I wonder why do they need build tool as runtime dependency?
~~Actually, It looks like a bug to me.~~
No, it's not a bug: mde/ejs#510 (comment)

TrySound · 2020-11-24T13:33:37Z

They use it to build cli mde/ejs@82a0309#diff-ca900686fca4680d4692bf587dc5da1322879c344688c187b5511cda27902c18R20

TrySound · 2020-11-28T18:30:36Z

-    ejs.renderFile(
-      `${projectRoot}/views/viewer.ejs`,
-      {
+    try {


Looks like there is no need in promise here anymore. The code is sync

It will be a breaking change.

I mean the function can be still async but promise wrapper around sync code is not necessary.

Ah, yep, agree. @realityking could you change it please?

Pushed a commit to do this :)

valscion · 2020-11-28T19:27:22Z

This approach is looking really promising to me! I'd be happy if we could make this pipeline easier by removing the ejs templates.

I'm thrilled to see your contributions, @TrySound and @realityking ❤️. I hope you're not feeling discouraged as we are a bit slow to look through your changes — rest assured that we are happy to see you contribute to webpack-bundle-analyzer 😊

EDIT: Oops, forgot to ping you both 😅

realityking · 2020-11-29T22:49:19Z

@valscion I was confused when I got the email 😄

So far, I'm really not concerned about review times. I realize nobody is working full-time on this. 🙂

th0r · 2020-11-30T09:12:18Z

Ok, let's merge this. I think removing an extra dependency outweighs the drawbacks. @valscion WDYT?

valscion

My thoughts: 🎉 😄

TrySound · 2020-11-30T11:13:25Z

+  if (openBrowser) {
+    open(`file://${reportFilepath}`, logger);
+  }
  });


this line should not be here I think

You're right. I wonder how the tests worked with this line present 🤔

They didn't. Travis hangs.

valscion

Yeah this looks good to me! If you feel like adding a changelog entry, that would be amazing ☺️

realityking · 2020-11-30T13:22:43Z

Done :)

valscion · 2020-11-30T13:46:44Z

I'll close and reopen the PR to see if Travis build would start up... I want to merge this PR 😄

realityking · 2020-11-30T13:50:28Z

@valscion It looks like webpack-bundle-analyzer is still on travisci.org. That will stop working entirely on December 31st but it might already cause performance issues: https://docs.travis-ci.com/user/migrate/open-source-repository-migration/#q-why-are-some-of-my-queued-jobs-taking-longer-than-usual-to-build

Switching to .com, or somewhere else entirely, would be a good idea.

alexander-akait · 2020-11-30T13:52:15Z

Maybe github actions is better idea, most of webpack repos already migrate on it

valscion · 2020-11-30T13:53:05Z

Maybe github actions is better idea, most of webpack repos already migrate on it

Yeah I have been wondering if github actions would be a good idea. A PR for that would be appreciated ☺️

valscion · 2020-11-30T13:53:24Z

I merged this PR now as I got tired of waiting on Travis.

realityking · 2020-11-30T13:54:19Z

Thanks @valscion :)

Let's hope I didn't overlook anything. Please ping in case master does error out and I'll fix it ASAP.

valscion · 2020-11-30T14:02:31Z

Released in v4.2.0 ☺️. Thank you for your contributions!

th0r requested changes Nov 24, 2020

View reviewed changes

Replace ejs templates with a simple js file

d44be8f

Add template tag for HTML to enable syntax highlighting

67e79b8

Fix missing semicolon

2b41272

TrySound reviewed Nov 28, 2020

View reviewed changes

th0r approved these changes Nov 30, 2020

View reviewed changes

valscion approved these changes Nov 30, 2020

View reviewed changes

Remove an unecessary promise wrapper

4c948e9

TrySound reviewed Nov 30, 2020

View reviewed changes

fix syntax error

11e4db1

th0r approved these changes Nov 30, 2020

View reviewed changes

valscion approved these changes Nov 30, 2020

View reviewed changes

Add changelog entry

084cc02

valscion closed this Nov 30, 2020

valscion reopened this Nov 30, 2020

valscion merged commit 302df4f into webpack:master Nov 30, 2020

realityking deleted the templates branch November 30, 2020 13:54

valscion mentioned this pull request Nov 30, 2020

Help us migrate CI to GitHub Actions #400

Closed

realityking mentioned this pull request Nov 30, 2020

Use polka instead of express to serve the viewer #212

Closed

Uh oh!

Conversation

realityking commented Nov 24, 2020

Uh oh!

valscion commented Nov 24, 2020

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!

th0r commented Nov 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

th0r commented Nov 24, 2020

Uh oh!

TrySound commented Nov 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

realityking commented Nov 24, 2020

Uh oh!

realityking commented Nov 24, 2020

Uh oh!

th0r commented Nov 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TrySound commented Nov 24, 2020

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!

valscion commented Nov 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

realityking commented Nov 29, 2020

Uh oh!

th0r commented Nov 30, 2020

Uh oh!

valscion left a comment

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!

valscion left a comment

Choose a reason for hiding this comment

Uh oh!

realityking commented Nov 30, 2020

Uh oh!

valscion commented Nov 30, 2020

Uh oh!

realityking commented Nov 30, 2020

Uh oh!

alexander-akait commented Nov 30, 2020

Uh oh!

valscion commented Nov 30, 2020

Uh oh!

valscion commented Nov 30, 2020

Uh oh!

realityking commented Nov 30, 2020

Uh oh!

valscion commented Nov 30, 2020

Uh oh!

Reviewers

Assignees

th0r commented Nov 24, 2020 •

edited

Loading

TrySound commented Nov 24, 2020 •

edited

Loading

th0r commented Nov 24, 2020 •

edited

Loading

valscion commented Nov 28, 2020 •

edited

Loading