Skip to content
This repository has been archived by the owner on Feb 24, 2018. It is now read-only.

Implement AST changes #16

Merged
merged 14 commits into from
Nov 10, 2015
Merged

Implement AST changes #16

merged 14 commits into from
Nov 10, 2015

Conversation

TimothyGu
Copy link
Member

@ForbesLindesay
Copy link
Member

I'm a little concerned someone might accidentally use this with a version of code-gen that has not been updated. We should update code-gen first and have it check explicitly for mustEscape === false so it fails into a safe state if paired with an out of date parser. Then I guess we could npm deprecate the old versions of jade-code-gen?

@TimothyGu
Copy link
Member Author

My understanding is that because the versions of these modules are <1, added the fact that a version of jade that uses these features has not been released, we can break compat in whatever way we want. I guess we should npm deprecate earlier versions. Then, when the AST is stable, we can then deprecate all the <1 versions.

@ForbesLindesay
Copy link
Member

I agree in principle, but given that this is such a security critical feature I want to be extra careful :)

@ForbesLindesay
Copy link
Member

This looks good except for the filtered include. Once that's updated to match our discussion in pugjs/pug-ast-spec#3, please reassign this pull request to me and ping me to have another look.

@TimothyGu
Copy link
Member Author

@ForbesLindesay, updated. Note that I left some commented-out errors in place, which we can use when pugjs/pug#2155 is implemented.

@jeromew
Copy link

jeromew commented Nov 7, 2015

the Use YieldBlock for yield keywords makes me wonder if we shouldn't change this keyword for 2.0 as yield is now a reserved keyword in javascript and people may get confused

@TimothyGu
Copy link
Member Author

@jeromew I believe changing it is planned for the next release: pugjs/pug#2055

@@ -270,7 +263,7 @@ Parser.prototype = {
type: 'Code',
val: tok.val,
buffer: tok.buffer,
escape: tok.escape,
mustEscape: tok.mustEscape !== undefined ? tok.mustEscape : tok.escape,
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer just tok.mustEscape !== false here. That fails to safe. Alternatively (perhaps as well) we could add an assert(typeof tok.mustEscape === 'boolean') check before this. I want to ensure everyone is on a compatible version, and that if they aren't then it's not dangerous, rather than trying to necessarily just work.

@ForbesLindesay
Copy link
Member

This is looking pretty good apart from those few minor points.

@jeromew yes, 3.0.0 will deprecate or remove the yield keyword and replace it with a more powerful model of blocks.

@TimothyGu
Copy link
Member Author

@ForbesLindesay all fixed.

// If there is a block, just ignore it.
this.block();
// TODO: make this a warning
// this.error('Raw inclusion cannot contain a block; block ignored', 'RAW_INCLUDE_BLOCK', this.peek());
Copy link
Member

Choose a reason for hiding this comment

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

Why not just make this a hard error? The block has always been ignored in the past so it seems fine to remove it as we release this major version.

@ForbesLindesay
Copy link
Member

You can merge this once RawInclude + Block is a hard error.

TimothyGu added a commit that referenced this pull request Nov 10, 2015
Implement AST changes
@TimothyGu TimothyGu merged commit c6d13ed into master Nov 10, 2015
@TimothyGu TimothyGu deleted the 2124 branch November 10, 2015 16:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants