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

Template interpolation service and template sourcemap #1215

Merged
merged 36 commits into from Apr 23, 2019

Conversation

Projects
None yet
2 participants
@octref
Copy link
Member

commented Apr 22, 2019

High level changes:

  • Update HTML parser to be able to determine if position is inside directive interpolations (previously only detect {{}}), and add tests
  • Rough implementation of sourcemap. Now transformTemplate and preprocess generate the from and to part of the sourcemaps
  • templateMode now has 2 parts, htmlMode (old html functionalities) and interpolationMode (new interpolation functionalities)
  • Refactor serviceHost and make it available to interpolationMode
  • interpolationMode can map back requests / responses

Smal changes:

  • Add test:lsp script to only run lsp tests

Todo:

  • Fix all integration tests for diagnostics interpolation
  • Add hover/definition/references and other functionalities to interpolationMode (this is rather straightforward once sourcemap is correct)

How source map works:

  • transformer parses all expressions
  • For each ThisKeyword created, add a from source map node
  • The transformed TS AST is printed via ts.createPrinter and re-parsed into SourceFile
  • Walk the new TS AST to find all this. access and add to source map node

Problem & Questions for the sourcemap:

  • For cases like foo, foo(), foo(5), foo(bar), foo = 5, the mapping becomes harder and harder. If I only map the this access, diagnostics errors might exist on 5 which is not mapped...
  • I think we should create unit tests for sourcemaps
  • @ktsn What do you think about this new approach / source map format?
    • Instead of trying to map on each this level, we try to map on ts.Expression level

    • Instead of the old map format like this (which don't work quite well once you need to map foo(5) to this.foo(5), since you have tokens before and after the foo)

      { from: { start: 0, end: 3 }, to: { start: 120, end: 123 } } // only for foo
      

      Create a new sourcemap like so:

      { from: { start: 0, end: 6 }, to: { start: 115, end: 126 }, thisDotRegions: { from: { start: 0, end: 0 }, to: { start: 115, end: 120 }
      

      This ensures that we can handle cases when initial interpolations have the this. inside it too.

      I don't know if we should try to generalize the source map work. It's so tedious that I think Vue template compiler should have a canonical version that everyone can depend upon.

@octref octref requested a review from ktsn Apr 22, 2019

@octref

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

Add hover/definition/references and other functionalities to interpolationMode (this is rather straightforward once sourcemap is correct)

Just to prove this is really trivial, I added interpolation hover in 5 minutes...This is rather easy to do. The hard part is getting the source map right.

image

@octref octref changed the title [WIP] Template service [WIP] Template interpolation service and template sourcemap Apr 22, 2019

@octref

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

Just occurred to me that ts.SourceFile has identifiers which is a list of all identifiers. Can we possibly use that to simplify injectThis?

@ktsn

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

@octref
Since transformed source file and printed source file have the exact same AST structure (don't they?), what about simply compare both AST and create source map from their node pos? Something like:

walkTSNode(sourceFile, newSourceFile, (from, to) => {
  if (from.pos === -1 || from.end === -1) {
    return;
  }

  sourceMapNodes.push({
    from: {
      start: from.pos,
      end: from.end
    },
    to: {
      start: to.pos,
      end: to.end
    }
  }
});

Instead of trying to map on each this level, we try to map on ts.Expression level

I think we need to do this. In any cases, there are variables without this. prefix which can provide diagnostics (e.g. v-for local scope).
If we can create source map by expression level, we can solve injected this. problem, right?

@ktsn

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

Just occurred to me that ts.SourceFile has identifiers which is a list of all identifiers. Can we possibly use that to simplify injectThis?

Does that get all declared identifiers in a source file? I'm not sure about that because we need to handle scope changes by v-for and v-slot.

@octref

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

what about simply compare both AST and create source map from their node pos?

This wouldn't work because the new source file has children set while the old one does't (difference between setting setParentNode in ts.createSurceFile./

OK, it seem I can still walk through the tree with ts.forEachChild once all nodes set their parents...https://github.com/Microsoft/TypeScript/blob/master/src/compiler/parser.ts#L873-L903

NVM, actually ts.forEachChild can traverse SourceFile of all synthetic nodes. I'll give it a try.

@octref

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

Some explorations...

ts.getSourceMapRange

    /**
     * Gets a custom text range to use when emitting source maps.
     */
    function getSourceMapRange(node) {
        var emitNode = node.emitNode;
        return (emitNode && emitNode.sourceMapRange) || node;
    }
    /**
     * Sets a custom text range to use when emitting source maps.
     */
    function setSourceMapRange(node, range) {
        getOrCreateEmitNode(node).sourceMapRange = range;
        return node;
    }

Here's a rough plan:

  • In template transform, use ts.setSourceMapRange(eslintASTRange) for synthetic expressions
  • Walk through the newSourceFile, look for two things:
    • If ts.getSourceMapRange call for the corresponding node in sourceFile has valid position
    • ThisKeyword inside PropertyAccessExpression. Mark those regions

So for foo(bar)

  • Transformed synthetic ts.Expression this.foo(this.bar) should have sourceMapRange as (0, 8), although the length doesn't match

  • When walking through the new SourceFile, see that the non-synthetic ts.Expression with range (0, 18) have a corresponding synthetic ts.Expression in sourceFile, with its sourceMapRange set to (0, 8)

  • Walk through the ts.Expression in the new tree. For each PropertyAccessExpression where its expression is of type ThisKeyword, mark its range for this.

  • A SourceMap node look like this:

    SourceMapNode {
      from: [0, 8],
      to: [0, 18],
      toThisDotRanges: [[0, 5], [9, 14]]
    }
    
  • Range [0, 4] of from are mapped to [5, 9] of to

  • Range [4, 8] of from are mapped to [14, 18] of to

@octref

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

  • Add references/definition
  • New SourceMap format
  • Separate template interpolation integration test with yarn test:int

OK, just two more test to fix then we are good.

  • v-on: This seems to fail because we generate :Event in a JS source type. Also Event can't be resolved — should we always include the dom lib for template language service?

  • object-literal: Suppose the original object literal is multi-line, such as

    {
      foo: bar
    }
    

    it'll be translated to { foo: this.bar }.

    Unfortunately the printer couldn't help removing the newlines. I also didn't find a way to write the original AST node so printer would keep the whitespace.

    That means we need to map each child of the ObjectLiteralExpression. All children should be in the same TemplateSourceMapNode — just need to generate offsetMapping differently.

@ktsn Why did you choose .JS source type? If we are gonna generate both js/ts, .TS is better as it accommodates both output, right?

@octref

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

  • Start a suite of SourceMap generation test Moved to #1218
  • One setting to enable/disable interpolation mode
@ktsn

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

Why did you choose .JS source type?

I remember this is to avoid noImplicitAny errors in template originally. But we don't need it anymore as we are now having separated service host for template interpolation.

@octref octref changed the title [WIP] Template interpolation service and template sourcemap Template interpolation service and template sourcemap Apr 23, 2019

@octref octref added this to the April 2019 milestone Apr 23, 2019

@octref octref merged commit f062895 into master Apr 23, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@octref

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

@ktsn Fixed all tests! Thanks a lot for reviewing.

@octref octref deleted the templateService branch May 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.