Generate fast change detector classes in Dart code - #1932

+1 for reusing the existing code template of JitChangeDetector!

Whoops, I updated this before seeing you comment to diverge from the pattern in the js change detection, but I'd like to lobby for switching the Js code to a more consolidated template also. What do you think @vsavkin ?

Did you figure out a strategy of testing generated detectors? We need to be sure that our change detection system are in sync.

Did you have thoughts on the code layout in change_detection_codegen.dart? Would you be okay with me updating the Js version to flatten out the class template in a similar way?

Re: testing, what I'm currently thinking is:

1. Create a serialization format for ChangeDetectorDefinition
2. Refactor current tests to extract those definitions
3. Create a gulp task that generates change detectors from the extracted definitions
4. Create a test mode that uses those generated classes to run the tests.

What do you think?

Refactor current tests to extract those definitions

This sounds good. Can you do it for just one simple test? So we can review it asap?

Just pushed updates to the JITGenerator & Dart Codegen class to make them parallel each other.

If you're okay with it, I'd like to get this checked in before starting to integrate the unit tests. Since the Dart change detectors aren't actually being used anywhere yet it shouldn't cause any problems, and the refactoring to integrate the two feels like it should be a separate commit to me.

@kegluneq #1983 has been merged into master, it introduces operator === & !==. Should the Dart codegen be updated to translate this to (!)identical(left, right) ?

We should probably use looseIdentical (link) to ensure good behavior with strings.

Are there updates that need to be made to this PR or will edits in ChangeDetectionUtil cover this support?

The actual change detector classes are being generated manually via a tool and the CREATE_ME gulp task needs to be cleanly integrated, but test.unit.js/ci and test.unit.dart/ci are running the tests and passing now -- we're getting close!

We can either try to automate generating the change detector classes or we can manually re-generate and check them in whenever the change detector tests are updated. I don't particularly like either option, but of them I'd vote for manually running -- automating this seems like it could get a little too clever.

FYI, we will update clang-format to a new version today (see #2059). So before this PR gets rebaed ontop of master, please do a local clang-format with the newest clang-format locally to minimize merge conflicts.

ACK for the clang-format

@vsavkin and I just discussed this and our plan for landing is

1. Code generation will not be part of the standard gulp pipeline, it will be run manually.
2. Generated code will be checked into a generated/ subdirectory.
3. Add a test to gulp test.unit.dart/ci that will fail if the generated/ files are out of date and output the command necessary to re-generate (like gulp check-format)
4. profit

This requires the least amount of magic and should allow anybody not updating the change_detector code to completely ignore the generated code.

I spoke with @vsavkin to get a bit more context and we agreed that it would be good to make the following changes:

• don't generate tests via a template, but instead use the json file to refer to the expression from hand written tests. This way, we don't need to worry about misgenerating tests (or not generating them at all) and we'd improve the debugging experience as well. Additionally, since many other tests can't be simply generated, we'll need to do this anyway, so we might as well do it for all tests rather than have two different code paths.
• don't store generated change detectors in the repo, but instead generate them during each build. This will ensure that we don't have stale code in the repo and that the codegen build tasks don't bit rot.

Okay, after discussing with @IgorMinar and @vsavkin this morning, here is the approach I'm thinking of:

Remove test generation via template & the JSON file. Test configuration is stored in a .ts file whose responsibility is to take a changeDetectorName: String and return a ChangeDetectorDescription object. For our simple watch checks, the name will simply be the constant expression we are evaluating. For more complex scenarios this will describe the feature we are testing.

Add a file defining a top-level function for each of dynamic, JIT, and PreGenerated that takes a ChangeDetectorDefinition and returns a ProtoChangeDetector. For dynamic and JIT these will be static and straightforward. For PreGenerated this will essentially map ChangeDetectorDescription#id to the appropriate generated ChangeDetector class.

In the tests, it will look like:

import {getDefinition} from './config';
import {createProtoChangeDetector} as dynamic from './dynamic';
import {createProtoChangeDetector} as jit from './jit';
import {createProtoChangeDetector} as pregen from './generated/simple_watch_classes';

function getProtoChangeDetector(cdType: string, def: ChangeDetectorDefinition) {
  switch(cdType) {
    case 'dynamic':
      return dynamic.createProtoChangeDetector(changeDetectorDef);
    // same for 'jit' and 'pregen'
  }
}

['dynamic', 'jit', 'pregen'].forEach((cdType) => {
  describe(`${cdType} Change Detector'), () => {
    it('should work for addition', () => {
      var changeDetectorDef = getDefinition('10 + 5');
      var changeDetector = getProtoChangeDetector(cdType, changeDetectorDef)
        .instantiate(dispatcher);
      expect(testAction(changeDetector)).toEqual('15');
    });
  });
});

@vsavkin , @IgorMinar I'm still not auto-generating the change detectors on each build but the test changes are done.

Please let me know if you have any issues with the approach.

I looked through the PR, the test changes look good to me. We still need to regenerate the detectors within the build. I'm not an expert on dart generators so I skipped that part.

I noticed that the expression definition is now in the TS file. how do you plan to extract the generators from there?

@IgorMinar , @vsavkin , the change detector build is now integrated into build/packages.dart and all tests are passing (locally, anyway).

PTAL

the gulp related bits lgtm. we'll clean it up more as we cleanup the gulpfile but this is good enough for now. thanks!

fyi, I have @IgorMinar signing off on gulp above & @vsavkin verbally gave the lgtm on the change detector code.

35f0ee5 introduced an issue that was causing transformer tests to fail -- fixing now & i'll submit once Travis is happy.

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}