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: internal getKaboomCtx and getInternalCtx, modulizate components #54

Merged
merged 9 commits into from
May 26, 2024

Conversation

lajbel
Copy link
Collaborator

@lajbel lajbel commented May 26, 2024

Okay, this is my new modulization idea

Because some components have to access to internal api (app, gfx) etc, I propose the following

getKaboomCtx()
getInternalCtx()

getKaboomCtx() gets the context from the global scope (the defined ctx variable) in kaboom.ts, with also the option of get the context from this.

For example, in a component

function sayHi() {
    // if is using multiple instances, will use this instance ctx
    const k = getKaboomCtx(this);
     
    return {
        hi() {
            k.debug.log("hi")
        }
    }
}

getInternal should work for the same, but the internal apis are not exported to any place, so it works as an array of internal contexts linked to the kaboom context, and when you use getInternalCtx, it finds the element with the context

function sayHi() {
    const k = getKaboomCtx(this);
    const internal = getInternalContext(this);
     
    internal.getViewportScale();
    
    return {
        hi() {
            k.debug.log("hi")
        }
    }
}

Let me know what do you think, for now I was trying to mantain compatibility with multipleboom example, so I think there's no breaking changes.

@lajbel lajbel added the refactor Changes structure of the project label May 26, 2024
src/kaboom.ts Outdated

// TODO: A better way to detect KaboomCtx
export const isKaboomCtx = (obj: any): obj is KaboomCtx => {
return Object.keys(obj).includes("loadAseprite");

Choose a reason for hiding this comment

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

This line doesn't look very nice. It can be tiring to convert the object to an array and check the keys one by one each time. If a component is added and removed frequently, this process will be repeated each time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, maybe only check obj["loadAseprite"]

@cemalgnlts
Copy link

Is this what you mean by context?

const k = kaboom({ global: false }) // One context.
const a = kaboom({ global: false }) // Another context.

@lajbel
Copy link
Collaborator Author

lajbel commented May 26, 2024

Is this what you mean by context?

const k = kaboom({ global: false }) // One context.
const a = kaboom({ global: false }) // Another context.

Yes!

https://kaboomjs.com/play?example=multiboom

@lajbel
Copy link
Collaborator Author

lajbel commented May 26, 2024

I changed more things, adding _k variable to KaboomCtx, this means future internal api (for now only getViewportScale) stuff will be exposed to the user.

In return, it is not necessary to use Array.find to get the internal api to be used in other files.

I don't know if it's a good thing to export the internal api, I'm thinking about the possibilities for plugin creators. But beyond that, does anyone have an opinion?

@cemalgnlts
Copy link

Good work, it looks better now. I have no idea at the moment what to do to make it better. Maybe this pull request can stay a little more open.

@lajbel lajbel changed the title refactor: internal getKaboomCtx and getInternalCtx refactor: internal getKaboomCtx and getInternalCtx, modulizate components May 26, 2024
@niceEli niceEli merged commit c4ff943 into master May 26, 2024
2 checks passed
@lajbel lajbel deleted the lj-modulizate-ctx branch July 7, 2024 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Changes structure of the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants