WIP: feat: handle concurrent updates to newly created record & upgrade deps

[jacobq]

This PR incorporates a number of changes (let me know if you'd rather I submit them separately):

1. Allows users to call record.save() on a record which has not yet finished saving without causing an error or a duplicate record to be created in the database. (See commit notes.) (Closes Cannot call .save() multiple times #239)

2. Fixes test suite race condition with store.unloadAll (Closes Add logging that hints at relationship bug ≥ ED3.5 #243)

3. Updates dependencies to current versions (dropping Node 6 support) and broadens test support matrix (Closes Add testing for LTS versions up to 3.8 #242)

4. Incorporates some minor refactoring (e.g. running some codemods for current ember conventions, replacing use of var with let and const, replacing tabs with spaces, etc.)

[jacobq]

I'm noticing that the test suite is somewhat broken. When I run ember test locally with node 12.x everything appears to pass. However, using npm test (ember-try) many scenarios fail. Looks like @backspace has started doing some work to update things in #242, and I'd like to help fix these.

I think we should also consider upgrading the minimum supported node version in the next semver major release from 6 to 10 since 6 is already at "end-of-life" status and 8 will be within the next few months.

This PR is going to need at least node 8.x because it uses ember-auto-import -> @embroider/core -> jsdom which requires node >= 8.

[jlami]

Are you still creating an uuid() for new records? I'm not seeing you call your new function generateIdForRecord.

And do you have a new test for the failing scenario? Without that it will be hard to really check the fix. And with just looking at the code I'm not entirely sure if the current promise setup will be correct. I believe that the id will only be set in ember-data after the createRecord promise is fulfilled. But your then on the promise we return to ember-data might be invoked before the chain that ember-data runs in their then really finishes. So ember-data might not be done with the first save() when your then is called for the second one. That is partly why I'm not really happy with ember-data just punting this issue to us.

While I'm really impressed by your refactors and do appreciate it, I'm not really sure if refactoring and fixing a bug should be done in the same pull request. The refactor makes it harder to view any real changes. So maybe you can break them up into two separate pull request?

[jacobq]

Are you still creating an uuid() for new records? I'm not seeing you call your new function generateIdForRecord.

It's not my function. It's part of the Ember Data Adapter API, and it seems to fit with what we're doing here.

If the globally unique IDs for your records should be generated on the client, implement the generateIdForRecord() method. ...

And do you have a new test for the failing scenario? Without that it will be hard to really check the fix. And with just looking at the code I'm not entirely sure if the current promise setup will be correct.

For now I have been using the repro I linked in #239 and the associated commit in this PR does fix that. I can add a test to the suite here before we land this (haven't done it yet because what was in the suite already wasn't passing in all cases).

I believe that the id will only be set in ember-data after the createRecord promise is fulfilled.

That's how it works if ember-data believes that the ID is being created on the server but not the case when generateIdForRecord is implemented.

While I'm really impressed by your refactors and do appreciate it, I'm not really sure if refactoring and fixing a bug should be done in the same pull request.

I'll be happy to cherry-pick things into separate PRs if you like. When I ran into trouble with the test suite (esp. when trying to update deps, etc.) I just started using this as my dev branch.

The refactor makes it harder to view any real changes.

FYI you can filter the changes by commit (e.g. here's just the changes for the fix)

[jlami]

It's not my function. It's part of the Ember Data Adapter API, and it seems to fit with what we're doing here.

That seems very useful. Good catch.

I can add a test to the suite here before we land this (haven't done it yet because what was in the suite already wasn't passing in all cases).

With the id generation out of this part of the code, that might not be as needed, but could always help.

FYI you can filter the changes by commit (e.g. here's just the changes for the fix)

I did look at this commit separately, but I guess I didn't read your commit message explaining the generateIdForRecord there and I may just be affraid some changes might sneak in with other commits. But not completely trusting your commit cleanlyness might say more about my own commit quality than yours ;)

Keep up the good work!

[jacobq]

@broerse @jlami @backspace If you have a few minutes I would appreciate your feedback on this:

• Does the solution to the "pineapple, pineapple" test failure look good? (wrapping store.unloadAll() in run)
• Does the chaining onto the adapter.createRecord promise seem like a reasonable solution for preventing errors or duplicate entries when record.save() is called again before it has finished persisting for the first time?
• Can you think of a better way to do this?
• What are your thoughts about dropping support for Node 6 (and perhaps 8 also) and cutting a new semver major release?

[backspace]

I was about to submit a PR myself for the pineapple, pineapple problem with this code that you suggested, as it did at least fix the tests for me locally:

  }).then(() => {
    this.store().unloadAll();

    return promiseToRunLater(1);
  }).then(() => {
    return this.store().findRecord('taco-soup', 'C');

But if your way works, fine with me.

I do have a preference for PRs to be separated more granularly. I think the test fix should be separate at least. Dropping Node 6 and 8 support is fine with me, but I think that would also belong in a separate PR. But I don’t know that we have any official policy on this, it’s just my own practice.

I’ll have to look at your other two bullet points more closely later.

[jacobq]

I do have a preference for PRs to be separated more granularly. I think the test fix should be separate at least.

No problem, that's pretty common (esp. among projects that like to squash all the commits in a PR when merging). #247 makes minimal changes to expand test matrix and "get green" again.

If you like, I can take a shot at applying the "multiple save" fix using the current UUID function so that it can be applied without updating node (so that we can use ember-auto-import) and make that PR too. Then all that's left would be an upgrade/clean-up PR. (Update: Actually, pouchdb 7.1.1 requires level 5.0.1 which needs node >= 8 anyway, so I stuck with ember-auto-import in #249)

[jacobq]

FWIW, I've made a first pass at preventing document update conflicts due to concurrent updates by the same application instance. Need to clean-up and check tests but it seems to work, and you can view it here: https://github.com/jacobq/ember-pouch/tree/jrq-debug