Skip to content

Express refactor: Static file routes and templates #2246

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
Closed
wants to merge 1 commit into from
Closed

Express refactor: Static file routes and templates #2246

wants to merge 1 commit into from

Conversation

GirlBossRush
Copy link
Contributor

@GirlBossRush GirlBossRush commented Oct 28, 2020 •
edited
Loading

This PR continues the Expressification of code-server.

Progress

  • Refactors asset path helpers and removes csStaticPath concept
  • Fixed issue where JSON entities may be unescaped
  • Updated web manifest schema, route handler
  • Fixed issue where body background color is often out of sync on startup and after theme change
  • Added types to vscode route.
  • Adds /local route for authenticated static file serving

Closes #1292

@GirlBossRush GirlBossRush added the enhancement Some improvement that isn't a feature label Oct 28, 2020
@GirlBossRush GirlBossRush requested a review from nhooyr as a code owner October 28, 2020 21:18
@GirlBossRush GirlBossRush self-assigned this Oct 28, 2020
code-asher
code-asher requested changes Nov 3, 2020
View reviewed changes
Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

Handlebars + static combo-wombo! This looks great! ty again for taking care of this.

@GirlBossRush GirlBossRush force-pushed the handlebars-templates branch 3 times, most recently from 6fece46 to d8fe245 Compare November 4, 2020 18:25
code-asher
code-asher reviewed Nov 4, 2020
View reviewed changes
Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

Latest changes look great! There are still some unresolved comments (looks like they're being collapsed and hidden though).

src/browser/views/vscode/index.ts
}

// Then, observe the meta theme element for changes.
const themeElement = document.getElementById("monaco-workbench-meta-theme-color") as HTMLMetaElement
Copy link
Member

@code-asher code-asher Nov 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is neat! In the future we could even update local storage with this method instead of doing it in the patch. It's always a happy day when we can reduce the patch size.

@GirlBossRush GirlBossRush changed the title Express refactor: Handlebars templates Express refactor: Static file routes and templates Nov 5, 2020
@GirlBossRush
Refactor template string with handlebars.
34b52a0 
- Refactor static routing, template vars.
- Fixed issue where JSON entities may be unescaped
- Updated web manifest schema, route handler.
- Clean up issues surrounding static paths, types.
- Removed static path helper.
- Fixed issue where locals may change during request handling.
- Added missing types.
- Fixed issue where body theme color is stale.
- Refactor missing tar endpoint.
- Add local route handler.
Base automatically changed from code-asher/ch1385 to master November 12, 2020 20:04
@code-asher
Copy link
Member

I haven't been able to re-review this yet. The divergence is growing and I'm not totally sure the best way to proceed but here's my plan:

  1. Close to keep the PR queue slim.
  2. Rebase on master.
  3. Split up the commit a bit.
  4. Open a new PR.

Additionally, we had a discussion about Handlebars and we want to try taking a route that lets us have type checking in the template (if we do though it'll basically just be replacing the .hbs files, so not a huge departure).

@code-asher code-asher closed this Nov 30, 2020
@nhooyr nhooyr deleted the handlebars-templates branch January 8, 2021 00:55
@nhooyr nhooyr restored the handlebars-templates branch January 8, 2021 00:56
@nhooyr
Copy link
Contributor

nhooyr commented Jan 8, 2021

Deleted this branch for now. I have it locally and saved on my fork in case we need to access these commits again. Just trying to keep the main repositories branches clean.

@nhooyr nhooyr deleted the handlebars-templates branch January 8, 2021 00:57
@nhooyr
Copy link
Contributor

nhooyr commented Jan 8, 2021

For posterity, the link is https://github.com/nhooyr/code-server/tree/z/handlebars-templates

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

@code-asher code-asher code-asher requested changes

@nhooyr nhooyr Awaiting requested review from nhooyr

Assignees

@GirlBossRush GirlBossRush

Labels
enhancement Some improvement that isn't a feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GirlBossRush @code-asher @nhooyr