Skip to content
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

Replace ejs templates with a simple js file #397

Merged
merged 6 commits into from Nov 30, 2020

Conversation

realityking
Copy link
Contributor

@realityking realityking commented Nov 24, 2020

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
Copy link
Member

@valscion valscion commented Nov 24, 2020

Please fix lint errors ☺️

src/template.js Outdated
</html>`;
}

exports.renderViewer = renderViewer;
Copy link
Collaborator

@th0r th0r Nov 24, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@realityking realityking Nov 24, 2020

Choose a reason for hiding this comment

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

Done

src/template.js Outdated
}

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

@th0r th0r Nov 24, 2020

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@th0r th0r Nov 24, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@realityking realityking Nov 24, 2020

Choose a reason for hiding this comment

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

  • 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
Copy link
Collaborator

@th0r th0r commented Nov 24, 2020

This drops 15(!) packages from the dependencies

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

@th0r
Copy link
Collaborator

@th0r th0r commented Nov 24, 2020

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

@TrySound
Copy link
Contributor

@TrySound TrySound commented Nov 24, 2020

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

@realityking
Copy link
Contributor Author

@realityking realityking commented Nov 24, 2020

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
Copy link
Contributor Author

@realityking realityking commented Nov 24, 2020

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
Copy link
Collaborator

@th0r th0r commented Nov 24, 2020

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
Copy link
Contributor

@TrySound TrySound commented Nov 24, 2020

src/viewer.js Outdated
ejs.renderFile(
`${projectRoot}/views/viewer.ejs`,
{
try {
Copy link
Contributor

@TrySound TrySound Nov 28, 2020

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@th0r th0r Nov 30, 2020

Choose a reason for hiding this comment

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

It will be a breaking change.

Copy link
Contributor

@TrySound TrySound Nov 30, 2020

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@th0r th0r Nov 30, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@realityking realityking Nov 30, 2020

Choose a reason for hiding this comment

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

Pushed a commit to do this :)

@valscion
Copy link
Member

@valscion valscion commented Nov 28, 2020

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
Copy link
Contributor Author

@realityking realityking commented Nov 29, 2020

@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
th0r approved these changes Nov 30, 2020
@th0r
Copy link
Collaborator

@th0r th0r commented Nov 30, 2020

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

Copy link
Member

@valscion valscion left a comment

My thoughts: 🎉 😄

src/viewer.js Outdated

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

@TrySound TrySound Nov 30, 2020

Choose a reason for hiding this comment

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

this line should not be here I think

Copy link
Contributor Author

@realityking realityking Nov 30, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@TrySound TrySound Nov 30, 2020

Choose a reason for hiding this comment

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

They didn't. Travis hangs.

th0r
th0r approved these changes Nov 30, 2020
Copy link
Member

@valscion valscion left a comment

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

@realityking
Copy link
Contributor Author

@realityking realityking commented Nov 30, 2020

Done :)

@valscion
Copy link
Member

@valscion valscion commented Nov 30, 2020

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

@valscion valscion closed this Nov 30, 2020
@valscion valscion reopened this Nov 30, 2020
@realityking
Copy link
Contributor Author

@realityking realityking commented Nov 30, 2020

@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
Copy link
Member

@alexander-akait alexander-akait commented Nov 30, 2020

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

@valscion
Copy link
Member

@valscion valscion commented Nov 30, 2020

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 valscion merged commit 302df4f into webpack-contrib:master Nov 30, 2020
1 of 2 checks passed
@valscion
Copy link
Member

@valscion valscion commented Nov 30, 2020

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

@realityking
Copy link
Contributor Author

@realityking realityking commented Nov 30, 2020

Thanks @valscion :)

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

@realityking realityking deleted the templates branch Nov 30, 2020
@valscion
Copy link
Member

@valscion valscion commented Nov 30, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants