GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Correctly cache tagged template objects in modules by DanielRosenwasser · Pull Request #18300 · microsoft/TypeScript

Conversation

@DanielRosenwasser

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) commented Sep 7, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

This PR is a work in progress at the moment

Remaining things to work on:

  • Bikeshed on helper name.
  • Bikeshed on helper semantics
  • Bikeshed on template object initialization location
  • Add helper for tslib
  • Add test for tslib
  • Investigate adding behavioral tests
  • Fix weird case with un-parenthesized new expressions

Fixes #17956

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Sep 7, 2017
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) commented Sep 7, 2017
View reviewed changes
Comment thread tests/baselines/reference/taggedTemplateStringsWithManyCallAndMemberExpressions.js Outdated
};
var f;
var x = new new new (_a = ["abc", "def"], _a.raw = ["abc", "def"], f(_a, 0)).member("hello")(42) === true;
var x = new new new f(_a || (_a = __getTemplateObject(["abc", "def"], ["abc", "def"])), 0).member("hello")(42) === true;

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Sep 7, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a regression. Ron Buckton (@rbuckton), any idea why we don't auto-parenthesize here?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) requested review from Ron Buckton (rbuckton), Nathan Shively-Sanders (sandersn) and Wesley Wigham (weswigham) September 7, 2017 03:28
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) mentioned this pull request Sep 7, 2017
Tagged template literals compiled incorrectly, TemplateStringsArray not cached #17956
Closed
Daniel Rosenwasser (DanielRosenwasser) added 2 commits September 6, 2017 20:39
@DanielRosenwasser
Update emit for tagged templates to use a per-site cached template ob…
c4b1a9d 
…jects.
@DanielRosenwasser
Accepted baselines.
166af8c
@DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) force-pushed the correctlyCacheTaggedTemplates branch from 587c8a8 to 166af8c Compare September 7, 2017 03:40
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) mentioned this pull request Sep 8, 2017
Design Meeting Notes, 9/8/2017 #18339
Closed
Daniel Rosenwasser (DanielRosenwasser) added 11 commits September 14, 2017 14:00
@DanielRosenwasser
Added printer test for 'new (f().x)'.
aa634ba
@DanielRosenwasser
Accepted (incorrect) baselines.
4beb9b0
@DanielRosenwasser
Parenthesize new'd expressions based on the leftmost node (or the fir…
6a9fa83 
…st node that is a call expression).
@DanielRosenwasser
Accepted baselines.
c966059
@DanielRosenwasser
Add a test case for conditional expressions just in case.
8fbb304
@DanielRosenwasser
Accepted baselines.
7871e08
@DanielRosenwasser
Explicit fall-through.
9f669d0
@DanielRosenwasser
Remove freezing behavior from tagged template helper.
e9c6dfe
@DanielRosenwasser
Accepted baselines.
1656790
@DanielRosenwasser
%s/getTemplateObject/makeTemplateObject
b137f24
@DanielRosenwasser
Accepted baselines.
5565709
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) mentioned this pull request Sep 18, 2017
Add the '__makeTemplateObject' helper. microsoft/tslib#39
Merged
@DanielRosenwasser
Merge branch 'master' into correctlyCacheTaggedTemplates
9907453
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) mentioned this pull request Sep 21, 2017
Incorrect emit for IIFE with assertion in property access in call expression #18668
Closed
@DanielRosenwasser
Merge branch 'master' into correctlyCacheTaggedTemplates
1cb5eb9
@msftgits Microsoft GitHub User (msftgits) removed the cla-not-required label Sep 26, 2017
Daniel Rosenwasser (DanielRosenwasser) added 4 commits September 27, 2017 17:07
@DanielRosenwasser
Added tests for import helpers with & without a declared template obj…
886a29b 
…ect creator.
@DanielRosenwasser
Ensure that the import helper is checked for tagged templates, and up…
1841afe 
…date its name.
@DanielRosenwasser
Accepted baselines.
0b7538d
@DanielRosenwasser
git Merge branch 'master' into correctlyCacheTaggedTemplates
b406d54
@DanielRosenwasser
Fall back to old behavior for tagged template emit in global files.
4ec1643
ghost
Deleted user (ghost) reviewed Sep 29, 2017
View reviewed changes
Comment thread src/compiler/checker.ts
}

function checkTaggedTemplateExpression(node: TaggedTemplateExpression): Type {
if (languageVersion < ScriptTarget.ES2015) {

@ghost Deleted user (ghost) Sep 29, 2017

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparent change in file permissions?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Sep 30, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/checker.ts
case ExternalEmitHelpers.AsyncDelegator: return "__asyncDelegator";
case ExternalEmitHelpers.AsyncValues: return "__asyncValues";
case ExternalEmitHelpers.ExportStar: return "__exportStar";
case ExternalEmitHelpers.MakeTemplateObject: return "__makeTemplateObject";

@ghost Deleted user (ghost) Sep 29, 2017

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not added in this PR, but all of the strings here are duplicated elsewhere in the emitter -- could we move this function to there?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Sep 30, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#18863

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/transformers/es2015.ts Outdated
let hierarchyFacts: HierarchyFacts;
let taggedTemplateStringDeclarations: VariableDeclaration[];
function recordTaggedTemplateString(temp: Identifier) {
const decl = createVariableDeclaration(temp);

@ghost Deleted user (ghost) Sep 29, 2017

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

taggedTemplateStringDeclarations = append(taggedTemplateStringDeclarations, createVariableDeclaration(temp));

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
DanielRosenwasser reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread src/compiler/transformers/es2015.ts Outdated
// Allocate storage for the template site object if we're in a module.
// In the global scope, any variable we currently generate could conflict with
// variables from outside of the current compilation.
const temp = isExternalModule(currentSourceFile) ? createTempVariable(recordTaggedTemplateString) : undefined;

@ghost Deleted user (ghost) Sep 29, 2017

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this variable be created closer to its use, or is there some side effect I'm missing?
If you do move it next to its use, you could just replace !temp ? ... with !isExternalModule(currentSourceFile) ? ...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
DanielRosenwasser reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread src/compiler/transformers/es2015.ts Outdated
text: `
var __makeTemplateObject = (this && this.__makeTemplateObject) || function (cooked, raw) {
if (Object.defineProperty) {
return Object.defineProperty(cooked, "raw", { value: raw });

@ghost Deleted user (ghost) Sep 29, 2017 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be more readable if both branches shared return cooked;:

if (Object.defineProperty) {
    Object.defineProperty(cooked, "raw", { value: raw });
} else {
    cooked.raw = raw;
}
return cooked;

Otherwise it looks like they return different things...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@weswigham Wesley Wigham (weswigham) Sep 29, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, you can go for even less readable, and do return Object.defineProperty ? Object.defineProperty(cooked, "raw", { value: raw }) : (cooked.raw = raw, cooked);. Can cram the entire helper into one line and it's not bad, actually:

var __makeTemplateObject = (this && this.__makeTemplateObject) || (cooked, raw) => Object.defineProperty ? Object.defineProperty(cooked, "raw", { value: raw }) : (cooked.raw = raw, cooked);

Too bad we don't transpile helpers 👅

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Sep 30, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spoke with Wesley; apparently Uglify can do a better job on the following thanks to ternary/comma precedence rules:

function __makeTemplateObject(cooked, raw) {
    if (Object.defineProperty) {
        Object.defineProperty(cooked, "raw", { value: raw });
    }
    else {
        cooked.raw = raw;
    }
    return cooked;
};

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/types.ts Outdated

FirstEmitHelper = Extends,
LastEmitHelper = ExportStar
LastEmitHelper = ExternalEmitHelpers[" LastPlusOne"] - 1

@ghost Deleted user (ghost) Sep 29, 2017 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also just move this next to the last one to make it obvious that these should be updated together. Same for FirstEmitHelper.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@rbuckton Ron Buckton (rbuckton) Oct 2, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. 👎 for the magic string in the enum.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
weswigham reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
sandersn
Nathan Shively-Sanders (sandersn) requested changes Sep 29, 2017
View reviewed changes

@sandersn Nathan Shively-Sanders (sandersn) left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to add a test that shows off the caching in action.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/types.ts
AsyncDelegator = 1 << 13, // __asyncDelegator (used by ES2017 async generator yield* transformation)
AsyncValues = 1 << 14, // __asyncValues (used by ES2017 for..await..of transformation)
ExportStar = 1 << 15, // __exportStar (used by CommonJS/AMD/UMD module transformation)
Extends = 1 << 0, // __extends (used by the ES2015 class transformation)

@sandersn Nathan Shively-Sanders (sandersn) Sep 7, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change is an example of why aligned comments make me sad :(

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Sep 30, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:(

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/factory.ts
return createParen(expression);

case SyntaxKind.NewExpression:
return (<NewExpression>emittedExpression).arguments

@sandersn Nathan Shively-Sanders (sandersn) Sep 28, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would swap this back to the original order.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/transformers/es2015.ts Outdated
// If we're in the global scope, we risk having conflicting variables.
// Since we currently lack the infrastructure to create sufficiently unique names,
// we'll fall back to creating the template object on every invocation.
templateArguments[0] = !temp ?

@sandersn Nathan Shively-Sanders (sandersn) Sep 29, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if isExternalModule is cheap to call, it would be nice to do so here; much easier to read.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@sandersn Nathan Shively-Sanders (sandersn) Sep 29, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also consider flipping the conditional.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
weswigham
Wesley Wigham (weswigham) requested changes Sep 29, 2017
View reviewed changes

@weswigham Wesley Wigham (weswigham) left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see a test that is downlevel, with modules, without importHelpers - just because I don't think I see any covering that configuration. Other than that, most everything else seems fine.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread ...s/baselines/reference/printerApi/printsNodeCorrectly.newExpressionOnConditionalExpression.js
@@ -0,0 +1 @@
new (x ? y : z) No newline at end of file

@weswigham Wesley Wigham (weswigham) Sep 29, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this a drive-by new test and fix?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@weswigham Wesley Wigham (weswigham) Sep 29, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes:

Fix weird case with un-parenthesized new expressions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@DanielRosenwasser
Accepted baselines.
d039942
@DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) force-pushed the correctlyCacheTaggedTemplates branch from 9f3db7f to f22a1fe Compare September 30, 2017 00:14
@DanielRosenwasser
Addressed code review feedback.
5da45fb
@DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) force-pushed the correctlyCacheTaggedTemplates branch from f22a1fe to 5da45fb Compare September 30, 2017 00:17
Daniel Rosenwasser (DanielRosenwasser) added 4 commits September 29, 2017 23:01
@DanielRosenwasser
Added test for module & global examples.
f94bded
@DanielRosenwasser
Accepted baselines.
81b3e85
@DanielRosenwasser
Updated helper for marginally better minification.
a23d1bf
@DanielRosenwasser
Accepted baselines.
e2c6aac
@rbuckton

Ron Buckton (rbuckton) commented Oct 1, 2017

Copy link
Copy Markdown
Contributor

I won't have time to look at this until Monday, but be aware that there was a Needs Consensus PR regarding template literal cache lifetime last week that reached consensus. I don't know yet whether this affects this change.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented Oct 2, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Ron Buckton (@rbuckton) within a module, this PR reflects those semantics. At the global scope, we continue to do the same thing as we've always done until we have a better mechanism.

Brandon CS Sanders (@sanders) Wesley Wigham (@weswigham) should be there now in tests/cases/compiler/taggedTemplatesInModuleAndGlobal.ts.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
rbuckton
Ron Buckton (rbuckton) suggested changes Oct 2, 2017
View reviewed changes
Comment thread src/compiler/transformers/es2015.ts Outdated
priority: 0,
text: `
var __getTemplateObject = (this && this.__getTemplateObject) || function (cooked, raw) {
if (Object.freeze && Object.defineProperty) {

@rbuckton Ron Buckton (rbuckton) Sep 7, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need the defineProperty call? I suppose it makes the property non-enumerable...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/types.ts Outdated

FirstEmitHelper = Extends,
LastEmitHelper = ExportStar
LastEmitHelper = ExternalEmitHelpers[" LastPlusOne"] - 1

@rbuckton Ron Buckton (rbuckton) Oct 2, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. 👎 for the magic string in the enum.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
weswigham reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread src/compiler/transformers/es2015.ts
scoped: false,
priority: 0,
text: `
var __makeTemplateObject = (this && this.__makeTemplateObject) || function (cooked, raw) {

@rbuckton Ron Buckton (rbuckton) Oct 2, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about:

cooked.raw = raw;
if (Object.freeze) Object.freeze(cooked);
return cooked;

Or, the one-line alternative:

return Object.defineProperty ? Object.defineProperty(cooked, "raw", { value: raw }) : (cooked.raw = raw, cooked);

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Oct 2, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uglify already produces a shorter output given this current implementation.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@weswigham Wesley Wigham (weswigham) Oct 2, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly better one-liner (two less characters, just as readable and correct):

return Object.defineProperty ? Object.defineProperty(cooked, "raw", { value: raw }) : cooked.raw = raw, cooked;

Which is, coincidentally, how uglify restructures what daniel has right now when it minifies it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/checker.ts
case ExternalEmitHelpers.AsyncDelegator: return "__asyncDelegator";
case ExternalEmitHelpers.AsyncValues: return "__asyncValues";
case ExternalEmitHelpers.ExportStar: return "__exportStar";
case ExternalEmitHelpers.MakeTemplateObject: return "__makeTemplateObject";

@rbuckton Ron Buckton (rbuckton) Oct 2, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth considering a shorter name than __makeTemplateObject?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Oct 2, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ron Buckton (@rbuckton) Babel uses _taggedTemplateLiteral which is two characters longer. If you can think of something by Monday afternoon before I merge this in, I will consider it 😄

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@weswigham Wesley Wigham (weswigham) Oct 2, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__template? (I like this since it doesn't have any camel-casing)
__templatize?
__intoTemplate?
__addRaw?
__createTemplateArg? (not really shorter, I know)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Daniel Rosenwasser (DanielRosenwasser) added 3 commits October 2, 2017 13:23
@DanielRosenwasser
Move first/last shortcuts closer to the end of the helper flags.
b80b2ee
@DanielRosenwasser
Flatten the '__makeTemplateObject' helper to use less vertical screen…
babe3cb 
… real estate.
@DanielRosenwasser
Accepted baselines.
8fd638c
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) changed the title Correctly cache tagged template objects Correctly cache tagged template objects in modules Oct 3, 2017
@DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) merged commit 301c90c into master Oct 3, 2017
@DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) deleted the correctlyCacheTaggedTemplates branch October 3, 2017 19:07
@microsoft Microsoft (microsoft) locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@weswigham Wesley Wigham (weswigham) weswigham requested changes
@sandersn Nathan Shively-Sanders (sandersn) sandersn requested changes
+2 more reviewers
@rbuckton Ron Buckton (rbuckton) rbuckton requested changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@DanielRosenwasser @rbuckton @sandersn @weswigham @msftclas @msftgits
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.