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

Change all apis on the Change, Data, all internal structures to be Symbols in order to not conflict with passed in data #122

Open
NullVoxPopuli opened this issue Jun 4, 2021 · 8 comments
Labels
enhancement New feature or request

Comments

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented Jun 4, 2021

Reproduction: https://runkit.com/nullvoxpopuli/reproduction-for-validated-changeset-122

for example,

import { Changeset as createChangeset } from 'validated-changeset';

let data = {
  trigger: { ...}
};

let changeset = createChangeset(data)

the data, trigger conflicts with: https://github.com/validated-changeset/validated-changeset/blob/ca0c7d249fc2244edaeb7aef25bdf06583628efa/src/index.ts#L144

I propose all internal APIS be assigned to private symbols instead of regular keys to not conflict with passed data:

example:

const TRIGGER = Symbol('__private__trigger__');

// ...
[TRIGGER](eventName: string, ...args: any[]): void {
  const notifier = notifierForEvent(this, eventName);

  if (notifier) {
    notifier.trigger(...args);
  }
@snewcomer
Copy link
Collaborator

This seems like a great idea! I'll get to working on this in the next few days!

@snewcomer
Copy link
Collaborator

Or of course, PRs are welcome!

@snewcomer
Copy link
Collaborator

@NullVoxPopuli A few thoughts.

  1. How does one supplant and override the entire trigger method with their own logic? Is this one tradeoff we have to deal with? Perhaps a more applicable example is if somebody wanted to describe how they determine isInvalid and do not want to use our get isInvalid.
  2. The real issue is we don't return the "getters/setters/methods" on the instance last. It happens somewhere before the end. I need to check the implications of pushing this as the last thing we do (and tests that may be impacted)

@NullVoxPopuli
Copy link
Contributor Author

  1. The symbols could be exported? So that way, if someone wants to subclass bufferedchangeset (ember), they could Import those symbols and define the appropriate overrides
  2. Understandable, but I think the approach of falling back to internal stuff last would then mean that passed in data could break behavior (unless symbols are used for internal stuff)

@NullVoxPopuli
Copy link
Contributor Author

Another possible approach is the change the api a bit, which would be breaking. The root of the issue right now is that changeset[property] proxies to the underlying data. If we forced using some other property and proxy that, problem solved. Similar to .get, except not using get.
Like, changeset.current.property

@snewcomer
Copy link
Collaborator

Class based inheritance should get us all the functionality we need. Prioritizing the following is probably all users need.

  1. Changes
  2. Content (original data pased to e-c)
  3. Changeset defined properties.

Can you help identify why the above prioritization might not work for a user? If we determine this is a bug (which I think it is - we prioritize Changeset over Content), then we can simply fix it.

We could use something like __methods = Symbol.for('ember-changeset-methods') and export that

export class EC {
  [__methods]: {
    toString
    trigger
    ...
  }
}

But my feeling is class based inheritance is sufficient in this case as long as we have the right prioritization order. Would love to hear your thoughts though!

#123

@snewcomer
Copy link
Collaborator

Well giving #123 a go!

@snewcomer snewcomer reopened this Jun 10, 2021
@snewcomer
Copy link
Collaborator

Alright I'm back to this issue 😆 . I completely borked the last PR b/c I thought my partner tests were passing. whispers they weren't.

I'll take a look at this during the week.

@SergeAstapov SergeAstapov added the enhancement New feature or request label Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants