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

Tag params #1076

Merged
merged 8 commits into from
Jul 10, 2018
Merged

Tag params #1076

merged 8 commits into from
Jul 10, 2018

Conversation

mlrawlings
Copy link
Member

@mlrawlings mlrawlings commented Jul 4, 2018

Description

Implements #656/#851

Allow tag bodies to receive parameters

<mouse-position(mouse)>
  Mouse is at ${mouse.x}, ${mouse.y}.
</mouse-position>

Parameters can be destructured, etc.

<mouse-position({ x, y })>
  Mouse is at ${x}, ${y}.
</mouse-position>

Feature Flag

We used to use this syntax to pass arguments to the custom tag and although this is now deprecated and replaced by spread attributes (#878), in order to avoid a breaking change this feature will live behind a feature flag until Marko 5.

marko.json

{
    "featureFlags": ["params"]
}

Checklist:

  • My code follows the code style of this project.
  • I have updated/added documentation affected by my changes.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Disclaimer: Contributions via GitHub pull requests are gladly accepted from their original author. Along with any pull requests, please state that the contribution is your original work and that you license the work to the project under the project's open source license. Whether or not you state this explicitly, by submitting any copyrighted material via pull request, email, or other means you agree to license the material under the project's open source license and warrant that you have the legal authority to do so.

@codecov
Copy link

codecov bot commented Jul 4, 2018

Codecov Report

Merging #1076 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1076      +/-   ##
==========================================
+ Coverage   90.53%   90.56%   +0.03%     
==========================================
  Files         305      307       +2     
  Lines       11636    11663      +27     
==========================================
+ Hits        10535    10563      +28     
+ Misses       1101     1100       -1
Impacted Files Coverage Δ
src/compiler/ast/FunctionDeclaration.js 100% <100%> (+2.43%) ⬆️
src/compiler/taglib-loader/loadTagFromProps.js 88.93% <100%> (+0.04%) ⬆️
src/compiler/ast/CustomTag.js 91.09% <100%> (+0.07%) ⬆️
src/compiler/util/parseJavaScriptParams.js 100% <100%> (ø)
src/compiler/Parser.js 91.54% <100%> (+0.2%) ⬆️
src/compiler/util/enableTagParams.js 100% <100%> (ø)
src/compiler/Builder.js 90.93% <100%> (+0.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ec6695...037d2f5. Read the comment docs.

@coveralls
Copy link

coveralls commented Jul 4, 2018

Coverage Status

Coverage increased (+0.03%) to 90.568% when pulling 037d2f5 on tag-params into 2ec6695 on master.

ok(typeof params === "string", '"params" should be a string');
ok(builder, '"builder" is required');

var parsed = builder.parseExpression("function(" + params + "){}");
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to parse this as an arrow function? Then the line numbers would already be right if we use them eventually.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because parseExpression converts to Marko AST and we don't have an arrow function node, it returns a generic Expression node (which doesn't have the params attached). So function it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

So I was concerned with some of the changes I made to parseJavaScript.js. By using esprima directly here I don't need to make those changes and can also use an arrow function.

@cameronbraid
Copy link
Contributor

Nice to see this work.. thanks.

Is there a way to globally defined a feature flag?

I don't currently rely on the old behaviour and I have many components that could make use of this feature but they don't currently have a tag def file for them

@cameronbraid
Copy link
Contributor

cameronbraid commented Jul 5, 2018

Sorry if this is off topic - could this same syntax be used for event delegation ?
mouse-position.marko :

class { 
  handleClick() {}
}
<${input} x=100 y=200 onClick=delegateTo('handleClick')/>
<mouse-position({ x, y, onClick })>
  Mouse is at ${x}, ${y}.
<div on-click(onClick)>click me</div>
</mouse-position>

would cause the transcluded on-click handler to delegate to the handler that mouse-position provides as 'onClick'

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.

4 participants