{"action":"created","issue":{"url":"https://api.github.com/repos/aurelia/aurelia/issues/281","repository_url":"https://api.github.com/repos/aurelia/aurelia","labels_url":"https://api.github.com/repos/aurelia/aurelia/issues/281/labels{/name}","comments_url":"https://api.github.com/repos/aurelia/aurelia/issues/281/comments","events_url":"https://api.github.com/repos/aurelia/aurelia/issues/281/events","html_url":"https://github.com/aurelia/aurelia/pull/281","id":380286706,"node_id":"MDExOlB1bGxSZXF1ZXN0MjMwNTIyMTIz","number":281,"title":"renderer refactor","user":{"login":"fkleuver","id":9655331,"node_id":"MDQ6VXNlcjk2NTUzMzE=","avatar_url":"https://avatars1.githubusercontent.com/u/9655331?v=4","gravatar_id":"","url":"https://api.github.com/users/fkleuver","html_url":"https://github.com/fkleuver","followers_url":"https://api.github.com/users/fkleuver/followers","following_url":"https://api.github.com/users/fkleuver/following{/other_user}","gists_url":"https://api.github.com/users/fkleuver/gists{/gist_id}","starred_url":"https://api.github.com/users/fkleuver/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/fkleuver/subscriptions","organizations_url":"https://api.github.com/users/fkleuver/orgs","repos_url":"https://api.github.com/users/fkleuver/repos","events_url":"https://api.github.com/users/fkleuver/events{/privacy}","received_events_url":"https://api.github.com/users/fkleuver/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":4,"created_at":"2018-11-13T15:37:29Z","updated_at":"2018-11-14T22:15:14Z","closed_at":null,"author_association":"MEMBER","pull_request":{"url":"https://api.github.com/repos/aurelia/aurelia/pulls/281","html_url":"https://github.com/aurelia/aurelia/pull/281","diff_url":"https://github.com/aurelia/aurelia/pull/281.diff","patch_url":"https://github.com/aurelia/aurelia/pull/281.patch"},"body":"<!---\r\nThanks for filing a pull request 😄 ! Before you submit, please read the following:\r\n\r\nSearch open/closed issues before submitting since someone might have pushed the same thing before!\r\n-->\r\n\r\n# Pull Request\r\n\r\n## 📖 Description\r\n\r\n<!---\r\nProvide some background and a description of your work.\r\n-->\r\nThis PR extracts the instruction-specific processing of the renderer out into one class per instruction, called `InstructionRenderers` (name could be better).\r\nWhat's left in the renderer itself is simple aggregation logic in the constructor that puts all registered instruction renderers into a `instructionRenderers` object map where the key is the instruction type (just like in the previous renderer), as well as the platform-agnostic loops that distribute the targets and instructions to the instruction renderers.\r\n\r\n### html-renderer object\r\nThe instruction renderers reside in html-renderer.ts and are exposed via an `HtmlRenderer` object that can be passed into DI. It contains the registrations for the renderers. The `all` DI injection function is used to inject a list of all registered instruction renderers into the `Renderer` class.\r\n\r\nIn effect, there is no more browser-specific code in the renderer or rendering engine and implementing alternative platforms like pixi or NativeScript should be a matter of registering a different set of instruction renderers (which may need to construct alternative bindings that use different observers, in some cases).\r\n\r\n### instructionRenderer decorator\r\nThere is a new decorator, `@instructionRenderer` which replaces the previous renderer extension point `@renderStrategy`.\r\nThe `renderStrategy` instruction type is also gone as it is no longer needed. `@instructionRenderer` can be used for both alternative environments as well as for simply extending the browser environment with custom rendering for custom `@bindingCommand`s.\r\n\r\nThe `@instructionRenderer` takes the instruction type (string) as an argument which becomes the instruction type string that this renderer will be able to handle.\r\n\r\n### 🎫 Issues\r\n\r\nDirectly related to: https://github.com/aurelia/aurelia/issues/182\r\nLoosely related to: https://github.com/aurelia/aurelia/issues/165\r\n<!---\r\n* List and link relevant issues here.\r\n-->\r\n\r\n## 👩‍💻 Reviewer Notes\r\n\r\nExample can be seen in `packages/jit/test/unit/binding-command.spec.ts` where there is a custom bindingCommand `\"keyup\"` with a corresponding same-name `instructionRenderer`, (was previously `renderStrategy`)\r\n\r\n### Renderer is instantiated/resolved differently\r\n`Renderer` is no longer constructed per template but instead registered via DI as a singleton. The underlying assumption is that you only need one set of renderers per Aurelia instance (this seems like a pretty safe assumption to make). To facilitate this the `RenderContext` is passed around via an additional method parameter instead of stored in the renderer, and the `RenderingEngine` uses DI to resolve the singleton `Renderer` instance.\r\n\r\nRender instructions also use DI to get only those specific dependencies that they need, such as parser/observer locator, etc. The hydrate instructions that need the renderer itself, use the RenderContext to resolve it.\r\n\r\n### InstructionType is set by the decorator\r\n`@instructionRenderer` replaces the constructor with a wrapper that sets the instruction type on the instance. This is effectively the same thing as `this.instructionType = 'x'`, but saves users that trouble (setting it on the instance is better for perf than setting it on the prototype because it's read very often)\r\n\r\n### An addition to BasicConfiguration\r\nThe `HtmlRenderer` object is added to the `BasicConfiguration` object in the JIT so for end users nothing additional needs to be done. Using a different renderer would mean using a different configuration that we would expose.\r\n\r\n### Fix in DI\r\nThe `all` function wasn't working correctly (called the wrong method), fixed that as well.\r\n\r\n<!---\r\nProvide some notes for reviewers to help them provide targeted feedback.\r\n-->\r\n\r\n## 📑 Test Plan\r\n\r\nA few small changes were needed to make all existing tests pass again. No additional tests were added because no functionality was added (a small `@instructionRenderer` specific test was already present in the JIT in the form of `@renderStrategy`)\r\n<!---\r\nPlease provide a summary of the tests affected by this work and any unique strategies employed in testing the features/fixes.\r\n-->\r\n\r\n## ⏭ Next Steps\r\n\r\n- Implement an additional renderer, such as pixi renderer or NativeScript (@bigopon could pick this up if he wants to, that would be a nice test to see if the extension point is good enough)\r\n- Proceed with completing the template compiler (surrogates) and adding an extension point there as well\r\n- Implement rendering of replace-parts?\r\n<!---\r\nIf there is relevant follow-up work to this PR, please list any existing issues or provide brief descriptions of what you would like to do next.\r\n-->\r\n\r\n<!--\r\nLove Aurelia? Please consider supporting our collective:\r\n👉  https://opencollective.com/aurelia\r\n-->\r\n"},"comment":{"url":"https://api.github.com/repos/aurelia/aurelia/issues/comments/438839030","html_url":"https://github.com/aurelia/aurelia/pull/281#issuecomment-438839030","issue_url":"https://api.github.com/repos/aurelia/aurelia/issues/281","id":438839030,"node_id":"MDEyOklzc3VlQ29tbWVudDQzODgzOTAzMA==","user":{"login":"bigopon","id":9994529,"node_id":"MDQ6VXNlcjk5OTQ1Mjk=","avatar_url":"https://avatars0.githubusercontent.com/u/9994529?v=4","gravatar_id":"","url":"https://api.github.com/users/bigopon","html_url":"https://github.com/bigopon","followers_url":"https://api.github.com/users/bigopon/followers","following_url":"https://api.github.com/users/bigopon/following{/other_user}","gists_url":"https://api.github.com/users/bigopon/gists{/gist_id}","starred_url":"https://api.github.com/users/bigopon/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bigopon/subscriptions","organizations_url":"https://api.github.com/users/bigopon/orgs","repos_url":"https://api.github.com/users/bigopon/repos","events_url":"https://api.github.com/users/bigopon/events{/privacy}","received_events_url":"https://api.github.com/users/bigopon/received_events","type":"User","site_admin":false},"created_at":"2018-11-14T22:15:14Z","updated_at":"2018-11-14T22:15:14Z","author_association":"MEMBER","body":"@fkleuver Lgtm. This is how i hoped it to be when mentioning decoupling the renderer. Not sure if this opened the opportunity for us to move the renderer to a level lower, aka completely separate all infra of instruction renderers. Atm, custom element and custom attribute are core assumptions, they could be hard to be separated. The main benefit of that is when doing integration with other platform, like pixi, phaser, native script or react natice, we may need very different custom element and custom attribute impl"}}
{"actor":{"display_login":"bigopon"}}