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

Reduce code or complexity #853

Closed
bonustrack opened this issue Oct 9, 2021 · 11 comments · Fixed by #912
Closed

Reduce code or complexity #853

bonustrack opened this issue Oct 9, 2021 · 11 comments · Fixed by #912
Labels
help wanted Extra attention is needed

Comments

@bonustrack
Copy link
Member

Reduce code, remove code duplicate, simplify things, remove unused dependency, if you can reduce code or complexity without removing a feature, feel free to make a PR.

@bonustrack bonustrack added help wanted Extra attention is needed 100 USDC labels Oct 9, 2021
@midgerate
Copy link
Contributor

midgerate commented Oct 11, 2021

Some examples.

  • Move duplicate code from snapshot-hub / snapshot to snapshot.js
  • Use vueuse useStorage https://vueuse.org/core/useStorage/
  • Replace ref and computed with $ref and $computed so we no longer need to use .value
  • Consider the possibility of using moment.js to work with dates and durations across the code.

@aiinkiestism
Copy link
Contributor

I'll work on this.

@mktcode
Copy link
Contributor

mktcode commented Oct 20, 2021

Consider the possibility of using moment.js to work with dates and durations across the code.

We agreed on day.js now: #887

@mktcode
Copy link
Contributor

mktcode commented Oct 20, 2021

Also related I'd say: snapshot-labs/snapshot-plugins#20 (comment)
And maybe: #818

@aiinkiestism
Copy link
Contributor

aiinkiestism commented Oct 21, 2021

@midgerate

Replace ref and computed with $ref and $computed so we no longer need to use .value

About this, I'm guessing you mean this.
I'm working on it but there are things that can't be replaced.
e.g. use not as a initializer like this.
Plus, declaration as a const is also impossible.
Both invoke error.

Below is my environment.
These are necessary for using Ref Sugar, but I still don't know the result of using much newer version of @vitejs/plugin-vue and @vue/compiler-sfc.

package.json

"@vitejs/plugin-vue": "^1.6.0"
"@vue/compiler-sfc": "^3.2.8"

vite.config.ts

...
plugins: [
    vue({
      // script: {
      //   refSugar: true
      // },
      refTransform: true
    }),
    ...

What do you think?
Btw, I'm just testing this on my local test branch so I can be flexible.

@aiinkiestism
Copy link
Contributor

I've just tested the latest version of @vitejs/plugin-vue and @vue/compiler-sfc.

package.json

---
  "@vitejs/plugin-vue": "^1.9.3",
  "@vue/compiler-sfc": "^3.2.20",
---

Two problems I mentioned in the last comment were still alive, so I think $ref and $computed will be of limited use and for the purpose of keeping code clean and consistent I'm not sure they are helpful right now, if my take is right.

@aiinkiestism
Copy link
Contributor

aiinkiestism commented Oct 23, 2021

@bonustrack @mktcode
snapshot-labs/snapshot-plugins#20 (comment)

About this, it seems to me that moving only plugin components to snapshot-plugins means lots of adding, changing of import because it's outside of the path unplugin-vue-components looking at, but is it good?
Sorry and please teach me how if I'm thinking the wrong solution or misunderstanding something.

@mktcode
Copy link
Contributor

mktcode commented Oct 23, 2021

Moving the UI components would be part of a more comprehensive refactoring of the plugin mechanic and should be handled in its own issue imho. (Adjusting the unplugin-vue-components config is indeed the approach I'd go for in that case.)

snapshot-labs/snapshot-plugins#20 (comment) is for now just a minimum approach to solve #666 but maybe also kind of a first step in such a refactoring. Just mentioned it as a potential example but maybe the scope of this issue needs to be narrowed down a bit more. Currently it's a bit "infinite".

@aiinkiestism
Copy link
Contributor

I'll just make PRs I've done for now Ready for review.

@midgerate
Copy link
Contributor

This is supposed to be a placeholder for documenting all the ideas for refactoring. There will be separate issues created in due time. Locking this thread.

@snapshot-labs snapshot-labs locked as off-topic and limited conversation to collaborators Oct 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants