Skip to content
This repository has been archived by the owner on Apr 29, 2023. It is now read-only.

Feat Vue3 and Typescript #78

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

Conversation

stefnotch
Copy link

@stefnotch stefnotch commented Jul 20, 2022

This is what the Vue 3 upgrade would look like.

Big changes that you'll probably notice

  • We've got a contributing guide now, check out CONTRIBUTING.md
  • No more Webpack, instead Vite is all the rage these days. It uses Esbuild under the hood, in case you're curious.
  • Vue 2 used the so called "options API". Vue 3 introduced the "composition API" and "script setup". Those are some super new concepts, since they basically change how the basic structure of the code looks. If you look at the files changed and scroll down, you'll find a few Vue files which have been changed.
  • Typescript
  • Vuex has been replaced by Pinia
  • The environment variables have been renamed a bit
  • There's an updated, and unused .proto file which is actually compatible with protobuf 3. Shoutout to CyberChef for making this somewhat easy. I think it might be worthwhile to fix all the protobuf stuff at some point.
  • We're now generating Javascript code and Typescript typings from the protobuf file.
  • There is a Typescript-y and more convenient IPC wrapper, with some type definitions in electron/ipc.ts
  • The flags code has been refactored a fair bit, braincells should be a bit happier now
  • Fix Ability to save the bank #26
  • Saving and loading a Javascript object from/to the disk. For saving larger banks, we either have to change the format to be an incremental one, or split it over multiple files, or use a proper database.

and some more stuff

Surprisingly not big changes

One thing that I didn't touch, but am curious about: Should we use style scoped? https://vue-loader.vuejs.org/guide/scoped-css.html

Update Vue code

More vue upgrading

Upgrade

Hm

Stuff

So many tweaks

Somewhat okay
Copy link
Author

@stefnotch stefnotch left a comment

Choose a reason for hiding this comment

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

Another thing you'll notice: Everything has been formatted with Prettier.

https://prettier.io/

@@ -0,0 +1,2 @@
VITE_APP_HOSTNAME=noita-together.unicast.link
VITE_APP_WS_PORT=6969
Copy link
Author

Choose a reason for hiding this comment

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

I probably shouldn't commit the .env file?

Copy link
Author

Choose a reason for hiding this comment

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

Also, those are the renamed env variables

"@typescript-eslint/no-explicit-any": "off",
"linebreak-style": "off",
},
};
Copy link
Author

Choose a reason for hiding this comment

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

Note that there's a second .eslintrc.js file in the src folder.

.gitattributes Outdated
@@ -0,0 +1,2 @@
# Use LF everywhere
* text eol=lf
Copy link
Author

Choose a reason for hiding this comment

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

Gotta have those sane line endings. The Windows style "carriage return" line endings are weird.

"Vue.volar",
"Vue.vscode-typescript-vue-plugin"
]
}
Copy link
Author

Choose a reason for hiding this comment

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

Those are the currently recommended Visual Studio Code plugins for working with Vue.
The old one, Vetur, isn't recommended anymore.

CONTRIBUTING.md Outdated


master branch: Electron app
mod branch: Noita together mod
Copy link
Author

Choose a reason for hiding this comment

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

Look, we got a bit of documentation!


const version = ref("");
ipcRenderer.invoke("get-version").then((v) => (version.value = v));
Copy link
Author

Choose a reason for hiding this comment

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

Oh, and beforeMount just isn't needed anymore. You just slap whatever you want to do right in the code and done.

function randomizeSeed() {
const seed = Math.floor(Math.random() * 4294967295) + 1;
payload.value.world.sync_world_seed.value = seed;
Copy link
Author

Choose a reason for hiding this comment

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

The hack here has been replaced with something nicer

<div class="info-wrapper" v-if="user">
<!-- TODO show personal alert for user -->
<Popper :hover="true" :interactive="true" class="info">
Copy link
Author

Choose a reason for hiding this comment

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

Instead of manually doing tooltips, I just used that library.

};
};

const useStore = defineStore("store", () => {
Copy link
Author

Choose a reason for hiding this comment

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

The store now looks way more like those new Vue components do.

  1. The state is just a reactive (and refs can also be used)
  2. Getters are computed()
  3. Mutations are plain old functions, and are an obsolete concept
  4. Actions are plain old functions


function commit(type: keyof typeof mutations, payload) {
return mutations[type](payload);
}
Copy link
Author

Choose a reason for hiding this comment

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

This commit function and the one below only exists so that I didn't have to refactor quite as much. Eventually it can be removed.

required bool locked = 7;
}
syntax = "proto3";
package NT;
Copy link
Author

Choose a reason for hiding this comment

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

What has changed in the protobuf file?
Well, first I had to move the syntax statement to the first line, so that it would get parsed at all.
Then, I had to shift nearly every single identifier by 1, so that nothing starts with zero.
And then I removed every single required, since proto3 apparently doesn't support those anymore.

Copy link
Author

Choose a reason for hiding this comment

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

The changes have been moved to a messages-updated.proto file. ^

@stefnotch stefnotch changed the title Feat vue3 Feat Vue3 and Typescript Jul 29, 2022
@stefnotch stefnotch marked this pull request as ready for review July 29, 2022 15:28
@stefnotch
Copy link
Author

One thing about the current design that I find odd is that the frontend store seems to duplicate some parts of noita.
Like the flags
https://github.com/soler91/noita-together/blob/master/src/noita.js#L40
https://github.com/soler91/noita-together/blob/master/src/store/index.js#L197

@stefnotch
Copy link
Author

Another tidbit that should probably be implemented: A loaded successfully message.

@soler91
Copy link
Owner

soler91 commented Aug 3, 2022

i appreciate the effort put into this, but i'd rather keep on the versions of tooling i'm familiar with so I can continue maintaining this project. could you pull the feature changes out into a separate PR that uses the current versions of tooling?

@stefnotch
Copy link
Author

stefnotch commented Aug 3, 2022

@soler91 Sure, I can do that. Though, I'll point out that the Typescript part of the tooling does let one catch a lot of mistakes. (See: Discord)
For instance, the fact that the current flags code worked at all was more luck than anything else.

The protobuf part is also pretty cursed at the moment, I'd love it if you could take a look at that eventually.

The Vue composition API is mainly in there, because it works slightly better with Typescript, and admittedly due to personal preferences. I could change that part back to the options API.

I can replace Pinia with Vuex again, shouldn't make much of a difference.

Replacing Webpack with Vite, upgrading Electron builder, etc. should all be rather unnoticeable changes.

@stefnotch
Copy link
Author

For posterity, there are a few bugfixes in this PR as well

Function not being called due to typo

I found another cursed little bug that has evaded detection so far.
Right here we've got a capital S

ServerPlayerSecretHourglass S_player_secret_hourglass = 33;

This is probably a typo. And that leads to
sPlayerSecretHourglass(payload) {
probably not getting called.

Modifying the default flags

The room flags code sometimes just returns the default flags

flags: (state) => {
const mode = state.room.gamemode
const fDefaults = state.defaultFlags[mode]
return fDefaults.map(flag => {
const found = state.roomFlags.find(f => f.id == flag.id)
if (!found && flag.type == "boolean") {
return { ...flag, value: false }
}
else if (found) {
return flag
}
else { return undefined }
}).filter(v => typeof v !== "undefined")
}

This can actually be observed: https://discord.com/channels/813249109208334386/813254974447419402/999373131904978965

  1. Create a room
  2. Turn a few flags off
  3. Leave the room
  4. Create a new room
  5. Instead of using the default settings, this new room now uses the changed flags

And also, it modifies the object returned by a Vue computed. This seems questionable to say the least.

flags.death[flag].value = true

Double escaping

The double escaping is probably really hard to observe, especially since the game doesn't aggressively throw errors.

toGame(obj = {}) {

Anyways, what happens there is:

  1. Let's input something like { event: "UpdateFlags" }
  2. JSON.stringify gets called and gives us the string "{\"event\":\"UpdateFlags\"}"
   const evt = JSON.stringify(obj)
  1. If there's no client, then we push it into the queue. So now the queue has "{\"event\":\"UpdateFlags\"}"
if (!this.client) {
            //console.log("[Game] Pushed code to queue.")
            this.queue.push(evt)
            return
        }

...
4. toGame is called again, and this time a client exists. We get to the final bit of code where our thingy is taken out of the queue and then we call toGame with it.

if (this.queue.length > 0) {
            setTimeout(() => {
                this.toGame(this.queue.shift())
            }, this.queueDelay)
        }
  1. toGame gets called with..."{\"event\":\"UpdateFlags\"}" . Yeah, that's a string.
  2. JSON.stringify, and we get a double-stringified thingy. "\"{\\\"event\\\":\\\"UpdateFlags\\\"}\""

Invalid protobuf file

The canonical protobuf implementation doesn't accept the protobuf file.
The documentation confirms some of the illegal bits: https://developers.google.com/protocol-buffers/docs/proto3

syntax = "proto3"; is the second line, which is wrong. It should be the first line

syntax = "proto3";

The first line of the file specifies that you're using proto3 syntax: if you don't do this the protocol buffer compiler will assume you are using proto2. This must be the first non-empty, non-comment line of the file.
-- https://developers.google.com/protocol-buffers/docs/proto3#simple

I'm guessing it's actually a proto2 file right now, since proto3 has other differences such as required not existing anymore protocolbuffers/protobuf#2497

And many of the field numbers start with zero, which doesn't seem to be legal either

The smallest field number you can specify is 1, and the largest is [...]
-- https://developers.google.com/protocol-buffers/docs/proto3#assigning_field_numbers

@stefnotch
Copy link
Author

Anyways, I'll leave this pull request open indefinitely, but I wouldn't expect this to get merged anytime soon (or ever).

@stefnotch
Copy link
Author

stefnotch commented Nov 17, 2022

(Oh neat, protobuf-ts is closer to being nice to use https://github.com/timostamm/protobuf-ts . They fixed one pretty big issue with the whole oneof stuff.)

@stefnotch
Copy link
Author

(Interesting, there is this ORM https://github.com/drizzle-team/drizzle-orm )

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

Successfully merging this pull request may close these issues.

Ability to save the bank
2 participants