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

resolveIdentifier regression issue after upgrading from 3.0.2 to 3.2.2 #1019

Closed
5 of 12 tasks
RainerAtSpirit opened this issue Sep 19, 2018 · 6 comments · Fixed by #1020
Closed
5 of 12 tasks

resolveIdentifier regression issue after upgrading from 3.0.2 to 3.2.2 #1019

RainerAtSpirit opened this issue Sep 19, 2018 · 6 comments · Fixed by #1020
Assignees
Labels
bug Confirmed bug has PR A Pull Request to fix the issue is available

Comments

@RainerAtSpirit
Copy link
Contributor

I have:

  • A conceptual question.
    • I've checked documentation and searched for existing issues
    • I tried the spectrum channel first
  • I think something is not working as it should.
    • I've checked documentation and searched for existing issues
    • I've made sure your project is based on the latest MST version
    • Fork this code sandbox or another minimal reproduction.
    • Describe expected behavior
      MST3.0.2 resolveIdentifier working:
      https://codesandbox.io/s/24zrq4n9n0 (models.CommentStore.js, line 15...)
 changeSelectedItem(uid: string) {
      // Working in MST3.0.2
      const item = resolveIdentifier(CommentModel, self.items, uid);
      console.log("item", item && item.toJSON());
      const item2 = self.items.find(i => i.uid === uid);
      console.log("item2", item2 && item2.toJSON());

      self.selectedItem = item;
    }
  • Describe observed behavior

Regression issue: After upgrading to MST3.2.2 resolveIdentifier returns undefined

https://codesandbox.io/s/294ylryp0

  • Feature request
    • Describe the feature and why you feel your feature needs to be generically solved inside MST
    • Are you willing to (attempt) a PR?

Not following the above template might result in your issue being closed without further notice

@RainerAtSpirit RainerAtSpirit changed the title resolveIdentifier regression issue after upgrading from 3.0.,2 to 3.2.2 resolveIdentifier regression issue after upgrading from 3.0.2 to 3.2.2 Sep 19, 2018
@xaviergonz
Copy link
Contributor

I took a look into it, it seems your id generator does not pass the test for the TOptionalId refinement

@xaviergonz
Copy link
Contributor

nevermind, it seems to be about the identifier being optional with a function, looking further into it

@xaviergonz
Copy link
Contributor

unit test

test("#1019", () => {
    function randomUuid() {
        return "1bbc17fd-2068-4cc9-aff0-82d37c9cdf20"
    }

    const CommentModel = types.model("CommentModel", {
        uid: types.optional(types.identifier, randomUuid)
    })

    const CommentStore = types
        .model("CommentStore", {
            items: types.array(CommentModel)
        })
        .actions(self => {
            function test() {
                const comment = CommentModel.create({})
                expect(comment.uid).toBe(randomUuid())

                self.items.push(comment)
                const item = resolveIdentifier(CommentModel, self.items, comment.uid)
                const item2 = self.items.find(i => i.uid === comment.uid)
                expect(item).toBe(item2)
            }

            return {
                test
            }
        })

    CommentStore.create({}).test()
})

@galdebert
Copy link

galdebert commented Sep 19, 2018

I had a similar (I think) issue today, a minimal repro is

  const Item = types
    .model({
      id: types.optional(types.identifier, 'dummykey'),
    })
  console.log(getIdentifier(Item.create())) // undefined, should be 'dummykey'

@xaviergonz xaviergonz self-assigned this Sep 19, 2018
@xaviergonz xaviergonz added bug Confirmed bug has PR A Pull Request to fix the issue is available labels Sep 19, 2018
@xaviergonz
Copy link
Contributor

The PR I just opened should fix both issues

@xaviergonz
Copy link
Contributor

should be fixed in the recently released 3.4.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug has PR A Pull Request to fix the issue is available
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants