-
Notifications
You must be signed in to change notification settings - Fork 648
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
Deprecate <script> with export and <script template-helpers> #547
Comments
What does moving static functions over to |
@mlrawlings Can you update this issue to also mention: static {
function myHelper() { ... }
var foo = 'hello';
} I wonder if it makes sense to recommend |
@mindeavor, @philidem and I had an impromptu discussion based on concerns raised in the Gitter chat room. The following proposal seemed to be liked by everyone: static {
function myHelper() {
/* ... */
}
class ViewModel {
constructor(data) {
this.data = data;
}
get fullName() {
return this.data.firstName + this.data.lastName;
}
}
}
component style lang="less" scoped {
.foo {
background-color: 'red';
}
}
component class {
onInput() { /* ... */ }
handleButtonClick() { /* ... */ }
} Reopening the issue for discussion. It's not too late to voice your approval or concerns. |
Although I find that syntax ok, my ideal syntax would still be something like this: <script marko>
import helper from 'library';
class Component {
onInput() { /* ... */ }
handleButtonClick() { /* ... */ }
}
function myHelper() {
/* ... */
}
class ViewModel {
constructor(data) { /* ... */ }
get fullName() { /* ... */ }
}
</script>
<style lang="less" scoped>
.foo {
background-color: 'red';
}
</style>
<div class="the-component">
<h1>${ state.name }</h1>
</div> |
I think it's not readable and clear like old syntax. Example if you are newbie in marko world and you see
You can easy understand that some logical operations is happen in script tags but if you see
You can think someone try to write in output "static function ... etc". Also if you need to write really to output something like this you need to wrap this code and it's not intuitive. I like marko html syntax and custom tags because it make me fill everything is standard with some extra power. Another thing why i like html syntax is if i have already written code in html it's really easy to integrate with my project without any other efforts but otherwise result can be unexpected. I don't like concise, mix syntax and maybe i am wrong. |
@Eldar-X @mindeavor @patrick-steele-idem @patrick-steele-idem
Would
|
I think it's good and unsurprising that marko transforms If we really want to stay away from <component>
import helper from 'library';
class {
onInput() { /* ... */ }
handleButtonClick() { /* ... */ }
}
function myHelper() {
/* ... */
}
class ViewModel {
constructor(data) { /* ... */ }
get fullName() { /* ... */ }
}
</component> My primary preference is to have a single tag for all my JS code if possible. |
@mindeavor I mostly agree (which is why this is the current behavior), but there is the concern that someone could be using Marko as a pure template language (without lasso, webpack, etc) — something we still want to support. Suppose you had a template like the following that you wanted to include in the <title>${data.title ? data.title + ' | Marko' : 'Marko'}</title>
<link rel="icon" type="image/png" sizes="32x32" href="/public/favicon.png" />
<meta name="viewport" content="width=device-width, initial-scale=1"/>
<style>
body { background:#f00; }
</style> It would probably be surprising if your
@Hesulan Yes that would be the equivalent HTML syntax, but I don't think that's a good idea (and Patrick is talking about potentially making that invalid).
It does not with the current implementation, but that would be trivial to implement. It would require updating the syntax highlighter though. |
I would like to point out that I prefer the HTML syntax, but I think it makes sense to use the concise syntax for these JS-like tags and other tags that don't directly affect the output, while still using the HTML syntax for tags that are actually involved in rendering the final HTML output.
Regarding For someone who is new to Marko, I really find it difficult to believe that someone would look at the following and not understand what is happening. And I don't see how wrapping this in a |
@mlrawlings I generally agree with what you're saying. The problem appears when you want something more than just a
The reason I was ok with Another proposal with these things in mind: script {
import helper from 'library';
class Component {
onInput() { /* ... */ }
handleButtonClick() { /* ... */ }
}
var languages = ['js', 'css', 'html']
function myHelper() {
/* ... */
}
class ViewModel {
constructor(data) { /* ... */ }
get fullName() { /* ... */ }
}
}
style {
.count {
font-size: 3rem;
padding: 0.5rem;
width: 3rem;
}
}
<button on-click('handleButtonClick') />
<select>
<option for(lang in languages) value=lang>lang</option>
</select> |
☝️ @patrick-steele-idem That does look nice. Other thoughts
@mindeavor I understand that concern and I could probably be convinced to dial back some of the JS tags introduced in v4, specifically Beside the fact that I like the component's class definition at the top-level, I really think that within the Here's my current thoughts on the list of "JS" tags we should support and their rational:
|
@mlrawlings I think dialing back the JS tags is a great solution. Having a minimal number of them makes me a lot more comfortable explaining and promoting the idea to others. I think I agree with all your points in tandem; your example suits well with me. The only confusion I foresee is people trying to The special tag for runtime code makes a lot of sense when you compare it to the If you don't mind a bit of bikeshedding... I think |
Totally fine. I have some more thoughts to share on the stylistic front. And would like your opinion on some of these things. One small issue I have with If you look at the OP, static var foo = 123; I'm curious what you think of this mode, but I like that it brings the statement up to the top level, but I still know that anything following the In any case, I wonder if we could have something that gives us a similar experience for the runtime JS. I still want something that could be (safely) interpolated within HTML. import { CurrencyFormatter } from './formatters';
static var format = new CurrencyFormatter('usd');
class {
constructor(input) { ... }
adjustPrice(index, amount) { ... }
}
% var products = state.products;
<table>
<tr for(i, product in products)>
% var name = product.name;
% var price = product.price;
<td>${ name }</td>
<td>${ format(price) }</td>
<td on-click('adjustPrice', i, -1)>- $1.00</td>
<td on-click('adjustPrice', i, +1)>+ $1.00</td>
</tr>
</table> In this case we would also support Okay, so I used We also looked at the following options: Looks nice, kinda like a REPL prompt, but could be confusing with $ var foo = 123;
// this has a space, so it's different than ${}
$ {
var bar = 456;
} Probably safe, but two characters: $$ var foo = 123;
$$ {
var bar = 456;
} Also looks like a prompt, but ruins any hope of using markdown in a template: > var foo = 123;
> {
var bar = 456;
} Only breaks nested blockquotes in markdown: >> var foo = 123;
>> {
var bar = 456;
} For reference:
|
To be honest, I'm ok with drawing the line at With that said, I don't know if |
@mindeavor Really appreciate your input on this. I do think we will end up in a good place and I think we all are in favor of reducing the number of special tags.
Agreed. Whichever syntax we finally settle on, it will be the same for both concise and HTML. We internally polled some devs at eBay and the following seems to be the most popular: |
Huh... I think I like it :) |
Would a backslash |
Yup, that's what I am thinking and that is what we allow if you need to escape placeholders. If you are good with it, then I propose we move forward with If anyone has any last minute objections please let us know! |
If we do, it will be a problem to put spaces after $ because most of our text editors can divide our code like this
Also i still think it should be at least for html version at least inside tags maybe like this
|
@patrick-steele-idem The only problem I see is that the Other than that I like this proposal. Though I do feel it's important to mark this as a breaking change, since anyone previously using something like |
Edit: Whoops, nevermind. For some reason I was thinking |
|
|
|
@mikewoo200 this past week I looked into what it would take to support linting of embedded JS inside Marko. We can do it in Atom very easily, but I think the main goal will be to ensure that the JavaScript is valid. In the future, when Marko supports source maps we can apply linting to the entire compiled output to catch problems such as undefined variables and unused variables (this cannot be done on individual embedded JS fragments). With source maps we can map the linter warning back to the location in the original template file. |
Here's another potential issue to discuss: Would any Example: <div>
$ console.log('x')
Price range is $ for this venue.
Price range is $$$ for that venue.
</div> |
That's the plan - first non whitespace character, followed by a space
…On Sun, Jan 29, 2017, 12:39 PM Gilbert ***@***.***> wrote:
Here's another potential issue to discuss: Would any $ enter "js mode",
or does the $ need to be the first non-whitespace character of its line?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#547 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AB3jnOk12Z1yIi79QgScEITg3Wpipjyfks5rXPkVgaJpZM4LtAQG>
.
|
Both need to be at the start of the line or prefixed only by whitespace:
|
@patrick-steele-idem, for "valid JS" linting (unused, undefined, etc.), what you mentioned would work. But for style linting (spaces, indents) in the template, what can we do? |
We could definitely come up with a way to enforce code style for embedded JS and Marko code as well. We'll probably need to explore that more after the v4 release. |
@patrick-steele-idem I'm a little unclear on the exact behavior of
do basically the same thing as I'm also not sure that
|
Copying thoughts from an internal discussion where we've decided So despite my initial reservations on It would only be allowed at the beginning of a line (
Ambiguity has been the reasoning against this, but looking at it again, even without syntax highlighting, I think it is pretty clear (and it definitely looks nicer), and @scttdavs has pointed out that with proper syntax highlighting it would be made very clear (red Part of the reason this makes sense is that we're already planning to allow $ var foo = {
bar: 1
} But JS already supports a block statement, so we would actually have to special case And there is an issue with <span.price>
$${price.toFixed(2)}
</span> So in order to get what you want in this case ( <span.price>
\$${price.toFixed(2)}
</span> But if you actually wanted to output the multiline sequence (perhaps you're explaining how to use it), you need to escape the second <code>
$\${
var price = 5;
}
</code> With the single <code>
\$ {
var price = 5;
}
</code> It's a fairly subtle difference, but I do think that it's only going to be an issue in one direction: // I could see someone doing this
${
var foo = 123;
var bar = 456;
}
// I don't see this happening
<div>
Hello my name is $ {name}.
</div> In the above case, we're typically going to see multiple JS statements because we also allow the single line version: $ var foo = 123; So, if you accidentally forget the space for a multiline JS block, it will compile to: out.w(escapeXML(var foo = 123;var bar = 456;)); That's going to be pretty easy to detect. |
@mlrawlings I like that, though I'm not sure what you mean by "special case Here's my interpretation of the parser logic:
If you need to enclose a multi-line tag's content in a javascript block statement, you'll simply need to use two pairs of brackets:
Optionally, the single-line syntax could also strip a single pair of surrounding brackets to avoid confusion. |
@Hesulan What I meant is that the single line version, because we allow continuing, is also the multiline version. So If we wanted to have a different symbol (like Your logic is pretty much how I implemented it: marko-js/htmljs-parser@2195823#diff-61eec70114a46820c77ce5879a128729R658 I'm going to close this as the work has been completed. Thank you everyone for your feedback and help in reaching this point! |
I'm late to the party, but I want to add my voice in support of not "concise" syntax. I do not think floating JS blocks are indicative enough of the code being associated with the template system. And as someone who abhors the "class" sugar, I'd be loath to use it solely for that reason. I would much rather see something like the following:
|
@jsumners I also dislike classes in general. However, the concept of a class fits perfectly with what a marko component is, and the class syntax is now officially part of the JS language. In marko's case, a class is the best tool for the job. As I understand it, the only "floating JS blocks" marko 4 will support are |
@jsumners I don't completely disagree - it would be nice to have a more HTML-like syntax - but after digging through the compiler code and giving it a lot of thought I'm personally convinced in favor of @patrick-steele-idem @mlrawlings On a related note, could |
@Hesulan
|
@patrick-steele-idem Yes, but could it fall back to parsing body-text if the block is omitted?
|
Hmm... do we only do it for <!-- Not quite JS -->
<class>
constructor() {
this.state = { count:0 };
}
increment() {
state.count++;
}
</class>
<!-- Highjacking the style tag again :( -->
<style>
button {
background:#fff;
}
</style>
<!-- the "concise" equivalent is technically a language construct, not a tag -->
<$ var foo = 123/>
<$>
var foo = 123;
var bar = 456;
</$>
<!-- I don't think I have any issues with these -->
<static var foo = 123/>
<static>
var foo = 123;
var bar = 456;
</static>
<!-- I *would* take issue with this -->
<static function sum(a, b) {
return a+b;
}/>
<!-- These are fine -->
<import foo from "file"/>
<export var num = 2/> |
@mlrawlings I only meant for |
We've decided to stop hijacking the script tag and use concise style tags that look like JS instead.
Deprecate
<script>
with exportOld:
New:
Deprecate
<script template-helpers>
Old:
New:
Moving to an external file
One concern you may have is that with the
<script>
with export approach, the only change required to move a component to an external.js
file was copy and paste. Admittedly, it's not quite that simple, but it's still really easy.This template:
becomes these two files
component.js
index.marko
So we copied over and had to add an export statement. Still pretty simple.
The text was updated successfully, but these errors were encountered: