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

Create background script #69

Merged
merged 6 commits into from
Jul 9, 2019
Merged

Create background script #69

merged 6 commits into from
Jul 9, 2019

Conversation

Bartleby2718
Copy link
Collaborator

1. Additional bundling through webpack-cli #43

  • Probably the most important part of this pull request.
  • After the CRA build(react-scripts build) is over, webpack is used to bundle node_modules and background.ts. The downside is that it takes twice as long as the previous build.
  • It uses a separate tsconfig file--tsconfig.other.json--to transpile TS files because it requires that noEmit be false. Still, I minimized the difference by extending the existing tsconfig.json. Simplify tsconfig.other.json through inheritance #68
  • In the process, I added ts-loader because it somehow works better than the existing awesome-typescript-loader.

2. Create folder upon installation #43

  • The code is basically the same as saveClicked or loadClicked because they all check the existence and the validity of webmarkFolderId.

3. Keyboard shortcuts #13

4. Refactorings #63 #66 #67

  • Thanks to utils.ts, it is now very clear what happens in the popup (index.tsx).

@Bartleby2718 Bartleby2718 added enhancement New feature or request refactoring Altering the internal--but not external--behavior labels Jul 8, 2019
@Bartleby2718 Bartleby2718 added this to the Minimum Viable Product milestone Jul 8, 2019
@jeongm-in jeongm-in self-requested a review July 8, 2019 15:24
Copy link
Owner

@jeongm-in jeongm-in left a comment

Choose a reason for hiding this comment

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

  1. Additional bundling
  • Need clarification on "works better." Does it mean performance improvement? Build time improvement?
  1. On install folder creation
  • confirmed folder is correctly created on install.
  • new folder is created in whatever occasion it is gone.
  1. Shortcut
  • confirmed shortcuts are functional.
  • also tested on Ubuntu 18.04.2 LTS with latest Google Chrome stable 75.0.3770.100-1, although it doesn't mean much.
  • will test on OSX environment later tonight.

@Bartleby2718
Copy link
Collaborator Author

Bartleby2718 commented Jul 9, 2019

  1. My bad. For some reason, I thought awesome-typescript-loader was included in the package but didn't work. It turns out it just wasn't installed. It works fine with awesome-typescript-loader once I npm install --save awesome-typescript-loader. Still, let's go with ts-loader since it it shorter and more popular on GitHub than awesome-typescript-loader, although I don't know about the performance difference.

2, 3. Thank you for the testings.

@jeongm-in
Copy link
Owner

Shortcut works fine as expected in OSX. Approve merge into master

@jeongm-in jeongm-in merged commit c21e0c9 into master Jul 9, 2019
@Bartleby2718 Bartleby2718 deleted the create-background-script branch July 12, 2019 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request refactoring Altering the internal--but not external--behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants