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.

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

Jump to bottom

Switching from jade to pug #21047

Merged
merged 8 commits into from
Jul 27, 2018
Merged

Switching from jade to pug #21047

merged 8 commits into from
Jul 27, 2018

Conversation

kobelb
Copy link
Contributor

@kobelb kobelb commented Jul 20, 2018

jade has been renamed to pug, so this is essentially a newer version of jade.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@kobelb
Copy link
Contributor Author

kobelb commented Jul 24, 2018

Jenkins, test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@jinmu03 jinmu03 mentioned this pull request Jul 25, 2018
Closed
@kobelb kobelb requested review from spalger and azasypkin July 25, 2018 11:44
@kobelb
Copy link
Contributor Author

kobelb commented Jul 25, 2018

@azasypkin are you familiar enough with the way that internationalization works to determine if this switch from jade to pug causes issues for them? My main concern is the following breaking change:

We removed support for interpolation in attributes since it was unnecessarily complex in implementation and tended to delay users learning that they can just use any JavaScript value in place of attributes

from pugjs/pug#2305

spalger
spalger approved these changes Jul 25, 2018
View reviewed changes
Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

I would prefer we transition completely to pug, converting any mention of jade in the i18n stuff, but what you have here looks good too.

package.json
@@ -140,8 +140,6 @@
"http-proxy-agent": "^2.1.0",
"https-proxy-agent": "^2.2.1",
"inert": "4.0.2",
"jade": "1.11.0",
"jade-loader": "0.8.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉 I somehow always forget to remove this loader when I'm messing around with the optimizer

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

yankouskia
yankouskia approved these changes Jul 26, 2018
View reviewed changes
Copy link

@yankouskia yankouskia left a comment

Choose a reason for hiding this comment

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

Excellent idea!
Just added one note.

src/dev/i18n/extract_default_translations.js Outdated
(paths, entry) => {
const resolvedPath = resolve(inputPath, entry);

if (resolvedPath.endsWith('.html')) {
paths.htmlEntries.push(resolvedPath);
} else if (resolvedPath.endsWith('.jade')) {
paths.jadeEntries.push(resolvedPath);
} else if (resolvedPath.endsWith('.jade') || resolvedPath.endsWith('.pug')) {
Copy link

Choose a reason for hiding this comment

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

since switching to pug, I would remove jade at all for avoiding misunderstanding

@azasypkin azasypkin removed their request for review July 26, 2018 16:10
@elasticmachine
Copy link
Contributor

💔 Build Failed

@kobelb
Copy link
Contributor Author

kobelb commented Jul 26, 2018

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@kobelb
Copy link
Contributor Author

kobelb commented Jul 26, 2018

A different set of flaky tests failed CI this time. Red + Red == Green?

@kobelb
Copy link
Contributor Author

kobelb commented Jul 26, 2018

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@kobelb
Copy link
Contributor Author

kobelb commented Jul 27, 2018

Different flaky tests fail each time. Gonna merge this anyway.

@kobelb kobelb merged commit 15e3e59 into elastic:master Jul 27, 2018
@kobelb kobelb deleted the pug branch July 27, 2018 11:21
@kobelb kobelb added v7.0.0 v6.5.0 non-issue Indicates to automation that a pull request should not appear in the release notes labels Jul 27, 2018
@kobelb kobelb mentioned this pull request Jul 27, 2018
Merged
kobelb added a commit to kobelb/kibana that referenced this pull request Jul 27, 2018
@kobelb
Switching from jade to pug (elastic#21047)
23cfca4 
* Upgrading pug

* Switching .jade to .pug and fixing templates

* Renaming the I18N usages of jade to pug

* No more jade in I18N
kobelb added a commit that referenced this pull request Aug 7, 2018
@kobelb
[6.x] Switching from jade to pug (#21047) (#21338)
c88b567 
* Switching from jade to pug (#21047)

* Upgrading pug

* Switching .jade to .pug and fixing templates

* Renaming the I18N usages of jade to pug

* No more jade in I18N

* Renaming root_redirect to .pug
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spalger spalger spalger approved these changes

@yankouskia yankouskia yankouskia approved these changes

Assignees
No one assigned
Labels
non-issue Indicates to automation that a pull request should not appear in the release notes v6.5.0 v7.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kobelb @elasticmachine @spalger @yankouskia