feat: vite, vue 3, pnpm, manifest v3, service_worker, etc #188
Conversation
feat: update to [email protected] feat: use pnpm feat: add typescript fix: memory leak of an event listener fixes simple-login#187
VividLemon
requested review from
acasajus,
cquintana92 and
nguyenkims
as code owners
|
|
||
| let that = this; |
There was a problem hiding this comment.
Oh yeah! Also, check out what this is, I'm not sure what the point of declaring this is. I thought this may have been to break reactivity in this function? But then I thought, what would be the point in that? I didn't do that in my version of the code -- there is no "this" context in script setup, so doing that is a bit difficult, and I didn't really understand the purpose.
Like, the values being used are in an event listener, so maybe the values needed are intentionally being used as raw values initialized in the event listener? But that would be super weird.
There was a problem hiding this comment.
That line is used to save the value of this in the current context and make it available in the following lines inside the this.onScrollCallback = async function () { block where that is used. Inside that block, the value of this is different from the value of this outside the block. We want the value of this outside the block, so we use that.
But you're right there is no "this" context in script setup so you can ignore that 👍
fix: remove dead TS from JS file
|
@VividLemon thanks for making this PR! Currently we are on another project and can only look at the PR next week, will let you know! |
|
@VividLemon I had the same error And thanks for your PR! It may not be fully ready yet as I saw some errors when trying to run it but it looks promising, hope you'll be able to start the extension and verify your changes :) |
|
I'm going to work on updating to Vite, but I need to work out on how to actually build a web extension. But among other things, there is an active bug in this code. |
|
Still working on this. Since I've never made an extension before, I had to learn a bit about how things work. I think I got everything figured out and what exactly is happening now. So, things should speed up on this |
VividLemon
changed the title
| <a href="https://chrome.google.com/webstore/detail/simplelogin-protect-your/dphilobhebphkdjbpfohgikllaljmgbn"> | ||
| <img src="https://img.shields.io/chrome-web-store/rating/dphilobhebphkdjbpfohgikllaljmgbn?label=Chrome%20Extension"> | ||
| </a> | ||
| # browser-extension |
There was a problem hiding this comment.
Revert this file. Accidental addition
| "description": "SimpleLogin Browser Extension", | ||
| "author": "[email protected]", | ||
| "license": "MIT", | ||
| "name": "browser-extension", |
There was a problem hiding this comment.
Metadata of this was removed. Revert!
| @@ -0,0 +1,67 @@ | |||
| #!/usr/bin/env node | |||
There was a problem hiding this comment.
This will require a ts compiler, probably best to just use node-ts
|
Clearly, I've been preoccupied lately. Perhaps at a later time I will attempt to complete this |
feat: update to [email protected]
feat: use pnpm
feat: add typescript
fix: memory leak of an event listener fixes #187
This does various things.
The update to Vue2.7 is a requirement for Vueuse.
The components were converted to script setup because they needed access to composition api.
The goal of this was to show a beginning of #186 .
The removal of v-clipboard and its replacement with @vueuse/core useClipboard is a minor change (though it may not seem like it with the script setup change, more on that later). The v-clipboard change is ultimately rather large in terms of it's overall affect. See, the package was included globally whereas useClipboard is imported individually. Meaning that it's treeshakable (It's an ESM package). This affects its overall footprint size. This is why I decided to target v-clipboard first.
TypeScript was included. Though its not actually used during the build process (yet). This was to add some type inferences for my ease of use.
Pnpm is now used instead of npm.
There are two major issues that stand out however.
In the https://github.com/simple-login/browser-extension#contributing-guide , I was never able to get the package to start. Meaning I could not verify if the changes that I made actually worked. I would always receive the error
Error: error:0308010C:digital envelope routines::unsupported. This occurred before, and after my changes, regardless on if I used npm or pnpm, with node 20.2.0, or the latest version of 20, I tried 18, and 16 as well.Three Vue files were affected. Two of them were fairly minimal, but Main.vue was a bit larger in terms of its footprint. Look at it carefully. Particularly the
loadAliaswas troublesome.contentElemwas an odd case, as it used document.querySelector, when it could have been a template ref before 🤔 odd choice imo. So I changed that one.It looks like loadAlias is a virtual scroller. Which could possibly be replaced with useVirtualList from vueuse as well. But I'd need more time with the lib as a whole.
upgradefunction was not marked asasync, but usedawait. I made it asyncand I left a TODO note in there as there's a function that's referenced
getAliasOptions, that's never defined. This should throw, which is odd because the affect should leavethis.loading(nowloading.value) in a permanentloading = truestate.The fix mentioned was fairly simple, just added it into a
onBeforeUnmounthookIf this gets reviewed, and the issue with starting the dev server gets sorted, expect more to come 🤗