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

Add a lexer option for the Parser #1392

Closed
wants to merge 1 commit into from
Closed

Conversation

mquandalle
Copy link

Hello,

I'm currently building a jade package for the meteor framework. I need to do some modifications of the Lexer object and then ask the parser to use my custom object. Is that already possible? If no is that something that could be added?

Thank you

@ForbesLindesay
Copy link
Member

What modifications do you need to make? Usually it shouldn't be necessary

@mquandalle
Copy link
Author

I've just published the code I've been working on: mquandalle/meteor-jade

I've rewritten the compiler to produce a Meteor-compatible Syntax tree. The Lexer modification goal is to define a "Component" object. Indeed in Meteor, because of its reactive nature, if, each, unless, with are all components (user can define its own) and you can't just provide a JavaScript function. You can check the wiki page.

The current code is early stage but it works, at least for the provided examples. For now I interpret the Mixin node as a Component but eventually I'd like to be able to define a custom node.

@ForbesLindesay
Copy link
Member

Are you using a custom parser and compiler as well then? At some point you might as well just fork the project?

@mquandalle
Copy link
Author

I use the Jade Parser, but a custom compiler. For now I've just copied/pasted this PR to [overwrite the parser](For now I've just).

I would prefer to not fork the project to easily benefit of jade future developments. But yes, it might be necessary at some point.

@ForbesLindesay
Copy link
Member

That link doesn't seem to work. Are you forking the parser as well? If you're not using the compiler you don't really gain much over just forking. Any minor build of ours could totally break your system.

What actual new syntax do you add? Couldn't you use the output of the existing parser and just replace the compiler? That's almost always going to be a better bet.

@mquandalle
Copy link
Author

Here is the correct link: parser.js

Any minor build of ours could totally break your system.

No because the package used a determined version (see here). That's why forking or not forking isn't a big deal.

Couldn't you use the output of the existing parser and just replace the compiler?

Yes, that was my plan. But actually it makes the compiler a bit more simple if the node already has the good type. The only change I made to the lexer is to define if, each, unless, with as components.

@mquandalle
Copy link
Author

By the way the option I'd like to add is similar to what you already have here.

@mquandalle
Copy link
Author

@ForbesLindesay?

@ForbesLindesay ForbesLindesay added this to the 2.0.0 milestone Apr 2, 2014
@ForbesLindesay
Copy link
Member

I think we should think about something along these lines for the 2.0.0 release, but it would be a breaking change so can't go in a 1.x release.

@mquandalle
Copy link
Author

Why is it a breaking change?

@ForbesLindesay
Copy link
Member

because anyone could legitimately have a local called lexer. It's not a big breaking change, but it's a breaking change.

Also, I suspect we might want to think harder about better support for jade derivatives.

@vendethiel
Copy link
Contributor

Yeah, else you end up with code like my Nephrite project

@mquandalle
Copy link
Author

@ForbesLindesay I'm probably missing something obvious but I still don't understand how "anyone could legitimately have a local called lexer". lexer is an option given to the Parser constructor, is there any reason to pass a custom name in this options dictionary?

@Nami-Doc I've used another workaround https://github.com/mquandalle/meteor-jade/blob/master/plugin/parser.js#L22

@mquandalle
Copy link
Author

@ForbesLindesay, sorry to ping you again. I think that the following change:

- this.lexer = new Lexer(this.input, filename);
+ this.lexer = new (this.options.lexer || Lexer)(this.input, filename);

is not a breaking change.

@ForbesLindesay
Copy link
Member

The problem is the jade.render and jade.renderFile both take a single object that represents both locals and options for the parser. This is a legacy thing, and not ideal. I'd prefer it if everyone just used jade.compile, but they don't.

@TimothyGu
Copy link
Member

@ForbesLindesay do you think the code can be merged into the v2 branch? (With proper documentation of course)

@ForbesLindesay
Copy link
Member

Yes

@ForbesLindesay
Copy link
Member

We should also add a warning if you use the order option at the moment.

@ForbesLindesay
Copy link
Member

Support for overriding all stages of the pipeline will be added as part of 2.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants