Sitelet https://github.com/primer/github-vscode-theme/pull/42
Skip to content

Small optimisations - #42

Merged
simurai merged 9 commits into
primer:masterfrom
dcastil:small-optimisations
May 20, 2020
Merged

Small optimisations#42
simurai merged 9 commits into
primer:masterfrom
dcastil:small-optimisations

Conversation

@dcastil

@dcastil dcastil commented May 17, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

While working on #40 I found a few small things which could make contributions easier. I created a PR directly instead of opening an issue because the changes could be made easily. If you don't want to go forward with this, feel free to close it. 😊

Here are the changes and their explanation:

  • Removed themes from git to make it clear that src/theme.js is source of truth and to prevent theme source and themes going out of sync. In my previous PR I made the changes directly on themes/dark.json through GitHub's web interface without noticing that was the wrong way to do it.
  • Added pre-launch task so that themes are built when starting extension. I also added a second Run Extension Without Build config to enable running extension as before.
  • Made npm start the default build task so that it can be started with shortcut cmd + shift + B.
  • Write both themes in parallel to speed up build.
  • Removed source files and node_modules from extension build to make extension bundle smaller.

@paramaggarwal

paramaggarwal commented May 18, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Yeah, makes sense to remove the generated files from git to prevent any confusion. This paired with a GitHub action to auto-publish the theme (when a new GitHub Release is created) will be really powerful.

@simurai

simurai commented May 19, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Tried it out locally. It seems to work fine for the first change, but then when making another change I get this:

> github-vscode-theme@1.1.1 build /Users/simurai/local/github/github-vscode-theme
> node src/index.js

(node:65346) UnhandledPromiseRejectionWarning: Error: EEXIST: file already exists, mkdir './themes'
(Use `node --trace-warnings ...` to show where the warning was created)
(node:65346) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:65346) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Anything wrong with my setup or does fs.mkdir("./themes") need some sort of check if it already exists?

@dcastil

dcastil commented May 19, 2020

Copy link
Copy Markdown
Contributor Author

I forgot to add the recursive option to prevent the function from failing if the directory exists. Thanks for pointing it out!

@dcastil

dcastil commented May 19, 2020

Copy link
Copy Markdown
Contributor Author

I also made the process fail if writing the themes fails. This makes sure that you get notified and the extension won't just open. I think I missed the error that way.

Screenshot 2020-05-19 at 19 10 52

simurai
simurai approved these changes May 20, 2020
View reviewed changes

@simurai simurai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok, I think this is good to 🚢 👍

@simurai
simurai merged commit fc39230 into primer:master May 20, 2020
@dcastil
dcastil deleted the small-optimisations branch May 20, 2020 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@simurai simurai simurai approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dcastil @paramaggarwal @simurai @dubesoftware