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

Small optimisations #42

Merged
merged 9 commits into from
May 20, 2020
Merged

Small optimisations #42

merged 9 commits into from
May 20, 2020

Conversation

dcastil
Copy link
Contributor

@dcastil dcastil commented May 17, 2020 •
edited
Loading

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

paramaggarwal commented May 18, 2020 •
edited
Loading

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

simurai commented May 19, 2020 •
edited
Loading

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

dcastil commented May 19, 2020

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

@dcastil
Copy link
Contributor Author

dcastil commented May 19, 2020

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

@simurai simurai left a comment

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