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

Removed priority queue in favor of FlatQueue #1157

Merged
merged 12 commits into from
Oct 26, 2024

Conversation

RyanGuild
Copy link
Contributor

@RyanGuild RyanGuild commented Oct 22, 2024

Description

Removed the PriorityQueue lib in favor of FlatQueue for generators.

Type of change

  • Refactoring / style

Versioning

I am not sure how to do the file hash

  • Version is updated
  • Changed files hash is updated (still need help with this one)

Copy link

netlify bot commented Oct 22, 2024

Deploy Preview for afmg ready!

Name Link
🔨 Latest commit 8dd84e1
🔍 Latest deploy log https://app.netlify.com/sites/afmg/deploys/671a4b8ff16eae00089fca69
😎 Deploy Preview https://deploy-preview-1157--afmg.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@RyanGuild
Copy link
Contributor Author

Hey how do I trigger another netlify build? Also should I be making draft PR's like this?

@Azgaar
Copy link
Owner

Azgaar commented Oct 22, 2024

Hello and thanks for the contribution!

We have flatqueue used in other functions, so we should be using it where we need good performance OR native array methods when it doesn't matter. I don't thing this class is really helpful, also it's very ineffective.

I fully support the modernization effort, but it's already in progress (see #842) and utilizing a bot different approach. There are some core issues that should be solved before switching to es6.

Netlify is automatically rebuilding on commit. But it looks there is a bug in your code, the class method is enqueue, but the method called is queue.

@Azgaar Azgaar self-requested a review October 22, 2024 20:35
@RyanGuild
Copy link
Contributor Author

RyanGuild commented Oct 23, 2024

The reason that this lib was bugging me is that I couldn't determine where it came from. I was working with

./jsconfig.json

{
    "include": ["modules/**/*", "libs/**/*", "utils/**/*", "components/**/*", "config/**/*", "./*.js"],
    "compilerOptions": {
        "checkJs": true,
        "types": ["./env.d.ts"],
        "esModuleInterop": true,
        "target": "ES2021",
        "module": "ES2020",
    }

./env.d.ts

import Rivers from "./modules/rivers.js"
namespace global {
  interface Window {
      Rivers: typeof Rivers
      ...other pretty well isolated modules
  }
} 

To try to get some IntelliSense working. This worked on all of the utils, which were utterly unmodified, and they just got dumped into the global namespace of the editor. It was the modules wrapped in IIFEs that didn't get adequately declared. Luckily, these are some of the best isolate systems, and so knowing I was way off the rails of looking into JQuery dialog, I just sort of dove into taking it apart, thinking I would find something small to PR eventually.

I was chugging along doing some ESM conversions purely as an experiment when I came to this aleaPRNG and PriorityQueue that only existed in their minified form declared into the es5 namespace with no window bindings. Typescript just couldn't parse the minified dependency. This was similar to the aleaPRNG declaration that was easy to find on GitHub, so I whipped up a quick ESM version because it looked like an excellent open-source library to modernize. After all, the maintainer hasn't committed to it in years. BTW have a present I made for you repo jsr (not for now, for the future, but I just see how heavily PRNG is used in your app). I didn't add it to the app in my test branch;

I just wired it up with

./env.d.ts

import { default as aleaType } from '@esm-alea/prng'
interface Window {
    aleaPRNG: typeof aleaType

./main.js

// @ts-expect-error
window.aleaPRNG = aleaPRNG

Just sorta shoving my types on top of the existing library. This left me with only PriorityQueue, but this lib was pretty un-parsable and didn't have any license pointing home.

So this leads me to the only question of this novel here. Should I just replace the 5 or 6 occurrences of this priority queue with regular arrays or flatqueue. Or am I totally missing the point and your telling me that that lib is flatqueue?

(PS please tell me to buzz off if I'm bugging you)

Edit 1:
After further review, I now think I get that you're trying to tell me to make PRs against #842 rather than master and that this change isn't helpful because you do plan to use that rewrite as a basis for smaller PRS

@Azgaar
Copy link
Owner

Azgaar commented Oct 23, 2024

Use flatqueue or native array methods depending on the use case. PriorityQueue can be safely removed, I don't remember its source so it's not even licensed properly.

@RyanGuild
Copy link
Contributor Author

sweet that makes sense!

@RyanGuild RyanGuild changed the title Removed priority queue in favor of simple array extension Removed priority queue in favor of FlatQueue Oct 23, 2024
@RyanGuild RyanGuild force-pushed the chore/remove-pqueue branch from f3f3869 to 1203140 Compare October 23, 2024 19:13
Copy link
Contributor Author

@RyanGuild RyanGuild left a comment

Choose a reason for hiding this comment

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

This is not done. Just sitting at the pub debugging and updating.

Don't merge there's a typo in there

modules/provinces-generator.js Outdated Show resolved Hide resolved
modules/cultures-generator.js Outdated Show resolved Hide resolved
modules/provinces-generator.js Show resolved Hide resolved
@RyanGuild
Copy link
Contributor Author

looks GTG

@Azgaar
Copy link
Owner

Azgaar commented Oct 24, 2024

Will do some tests and merge. Will try to complete tomorrow, please ping me if I forget :)

@@ -266,7 +266,7 @@ window.Zones = (function () {

if (!cost[e] || p < cost[e]) {
cost[e] = p;
queue.queue({e, p});
queue.push({e, p}), p;
Copy link
Owner

Choose a reason for hiding this comment

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

It's the only issue I see

@Azgaar Azgaar merged commit d7f5cae into Azgaar:master Oct 26, 2024
4 checks passed
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.

3 participants