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

refactor: wip #37

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

refactor: wip #37

wants to merge 24 commits into from

Conversation

Kompwu
Copy link
Contributor

@Kompwu Kompwu commented Jul 17, 2018

ts-ify & new boilerplate

Todo:

I've also added some githooks to automate some shit & lint commit.

@Ashish-Bansal What do you think? 🤔

ts-ify & new boilerplate
tsconfig.json Outdated
"ES5" /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017','ES2018' or 'ESNEXT'. */,
"module":
"commonjs" /* Specify module code generation: 'none', 'commonjs', 'amd', 'system', 'umd', 'es2015', or 'ESNext'. */,
// "lib": [], /* Specify library files to be included in the compilation. */
Copy link
Owner

Choose a reason for hiding this comment

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

Though I know it's still in wip. Do we need any of these features or you plan to remove them up in future ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need all of them.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh okay.

@Ashish-Bansal
Copy link
Owner

@Kompwu I don't have any experience with ts. Earlier I only had heard about it. Now I see its various features. Looks good to me! 👍

Sorry for the late reply!

@Ashish-Bansal
Copy link
Owner

Also I saw you removed yarn.lock. Any specific reason for that ?

@Kompwu
Copy link
Contributor Author

Kompwu commented Jul 26, 2018

@Ashish-Bansal I'm gonna add a new one soon.
Edit: Can you help me ?

@Ashish-Bansal
Copy link
Owner

@Ashish-Bansal I'm gonna add a new one soon.
Edit: Can you help me ?

@Kompwu Sure, how can I help you out ?

@Kompwu
Copy link
Contributor Author

Kompwu commented Jul 26, 2018

@Ashish-Bansal Backport: #18 pls :)
Edit: I'm sorry i mean #20

@Ashish-Bansal
Copy link
Owner

Ashish-Bansal commented Jul 26, 2018

@Kompwu

Back porting ? I don't get why we would like to backport something to #18 and which feature ?

Or did you mean porting individual toggle per tab feature to this branch ?

@Kompwu
Copy link
Contributor Author

Kompwu commented Jul 31, 2018

@Ashish-Bansal Almost done :)
Now... FuseBox or Parcel ?

@Ashish-Bansal
Copy link
Owner

@Ashish-Bansal Almost done :)

Awesome! :D

Now... FuseBox or Parcel ?

Parcel seems to be more flexible than fusebox and requires less configuration boilerplate code than webpack, providing a nice middle ground. So, I would say let's try out Parcel.

@Kompwu
Copy link
Contributor Author

Kompwu commented Jul 31, 2018

@Ashish-Bansal The problem is that with Parcel we can't configure anything, so to manage the manifest.json we'll have to handle it by hand.
But, I think with a "hook", we can easily get the hell through.
Something like that:

const fs = require('fs');
const manifest = require("./src/manifest.json");
const pkg = require("./package.json");

manifest["name"] = pkg["displayName"]
manifest["version"] = pkg["version"]
manifest['description'] = pkg['description'];
fs.writeFile('dist/manifest.json', JSON.stringify(manifest));

And we run it at the same time as Parcel.
But we'll have another problem, no hot reloading on manifest.json...
I don't really know how to do this, I guess there must be a library out there to do this easily.
I'll see what I can find out.

Edit: Okay, we can handle it with fs.watchFile.
Time to configure Parcel.

@Ashish-Bansal
Copy link
Owner

@Kompwu wow, parcel configuration is quite cleaner compared to webpack. Nice 👍

By the way, since we removed crx-webpack-plugin , can you please add npm script for packing up the extension ?

May be https://github.com/oncletom/crx would help.

@Kompwu
Copy link
Contributor Author

Kompwu commented Aug 5, 2018

@Ashish-Bansal I'll see what I can do.
BTW, can you start backporting #20 ?

@Ashish-Bansal
Copy link
Owner

Sure.

@Ashish-Bansal
Copy link
Owner

@Kompwu Did you try loading extension in the browser ? I can see the paths are wrong in the manifest.json due to which it fails to load.

@Kompwu
Copy link
Contributor Author

Kompwu commented Aug 5, 2018

@Ashish-Bansal Are you talking about the path for js ?
Just change it to ts.
I forgot to push the commit to fix that.

@Ashish-Bansal
Copy link
Owner

@Ashish-Bansal Are you talking about the path for js ?

Yeah. Also, icons doesn't seem to load due to some reason. Though I haven't checked the reason for that yet.

@Kompwu
Copy link
Contributor Author

Kompwu commented Aug 5, 2018

@Ashish-Bansal Apart from that, everything works for you?

I've also made some performance improvements ;)
Edit: Do you have Discord?

@Ashish-Bansal
Copy link
Owner

Ashish-Bansal commented Aug 5, 2018

@Kompwu No, it's throwing errors in background script.

Edit: Do you have Discord?

Just did setup. Please share your username.

Edit: Discord Channel link: https://discord.gg/Mm52n3Y

@Kompwu
Copy link
Contributor Author

Kompwu commented Aug 5, 2018

@Ashish-Bansal Okay, i'm close.
There's definition matching issue on: https://github.com/Kompwu/audio-only-youtube/blob/90dadaf0dfe84fa3a9b9d3946ca3c3c2e36f71f0/src/ts/background.ts#L94
Error in event handler for webRequest.onBeforeRequest/1: Error: Invocation of form webRequestInternal.eventHandled(string, string, string, function) doesn't match definition webRequestInternal.eventHandled(string eventName, string subEventName, string requestId, optional object response)
looks like it's passing a function to a function argument that expects an object...
I'm so sleepy, I'll keep going after I sleep!

@Kompwu
Copy link
Contributor Author

Kompwu commented Nov 4, 2018

@Ashish-Bansal What about #14?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Loading Preview images
2 participants