search

tastejs / todomvc

Helping you select an MV* framework - Todo apps for React.js, Ember.js, Angular, and many more
http://todomvc.com
Other
28.6k stars 13.77k forks source link

Polymer example has build step in it #1490

Open LoveAndHappiness opened 8 years ago

LoveAndHappiness commented 8 years ago

The polymer app seems to have a build step that concatenates (maybe vulcanizes?) every module into one giant build.elements.html along with a giant build.elements.js file.

As a newcomer it isn't clear how the app is actually build and structured, when the repository is cloned, compared to other examples, like angularjs or vuejs, where the structure is pretty clear.

I recommend removing the build step from the polymer example, so that newcommers to polymer are able to rebuild the todoapp, since it is posted for eductational purposes.

LoveAndHappiness commented 8 years ago

I also opened a question regarding this issue here: http://stackoverflow.com/questions/33296543/how-to-rebuild-polymers-todomvc-app

samccone commented 8 years ago

cc @addyosmani @ebidel

samccone commented 8 years ago

@LoveAndHappiness As to rebuilding... in the readme checkout this section https://github.com/tastejs/todomvc/tree/master/examples/polymer#making-updates

:)

ebidel commented 8 years ago

Vulcanizing a Polymer app is a best practice and we want to encourage folks to use this tool for performance reasons. TodoMVC is supposed to encourage that. While having a minified/built file is not the norm for these demos, it's an unfortunate necessity for us until HTML imports are native everywhere in browsers. Unfortunately, people also see these live demos and make FUD comparisons between frameworks. I want devs to know the RightWay to build an app.

See my response https://groups.google.com/d/msg/polymer-dev/GocWEmUP8zE/VjtscPbTDQAJ on how to update/dev the todomvc setup. If you switch elements.build.html to elements.html in the index file, all is well :)

ebidel commented 8 years ago

We should close this issue. Updating the sample is already documented.

LoveAndHappiness commented 8 years ago

@ebidel

As much as I love polymer and especially as much as I love your work and that of your team, I really feel that TodoMVC is here to encourage people to try these new libraries and frameworks so that devs, regardless of their skill level in the specific framework, can use TodoMVC as an introduction.

Even the TodoMVC website says

Study the syntax required for defining models, views and (where applicable) controllers and classes in the frameworks you're interested in and try your hand at editing the code to see how it feels using it first-hand.

A build step doesn't allow for editing the code to see how it feels, because there is the elements.build.js.

I understand your point of view that an app is only ready, if it is vulcanized at this point in time. And I agree and encourage you to make it obvious in the docs.

But I believe that the mission of TodoMVC is to give devs an understanding on how the app works, not necessarily how the app looks like when it is finished.

And at least my approach has always been to clone the repository and to rebuild the app and try my hand at editing the code to see how it feels. And right now that's not possible, because it is vulcanized and I don't know how to unvulcanize it.

I really hope I can change your mind on this matter, because I love polymer a lot and think it is a great piece of work and my recommendation would help devs get into the library easier.

ebidel commented 8 years ago

It's a two line change to remove usage of the vulcanized files :) https://github.com/tastejs/todomvc/blob/master/examples/polymer/index.html#L8-L9

If there's a way for todomvc.com to serve the built version and devs get the unbuilt version on checkout, that would make me happy. We could switch the default to elements.html instead of elements.vulcanized.html. But I'm not familiar with how the site deploy works.

LoveAndHappiness commented 8 years ago

I have to agree with you (@ebidel). It's a two line change to remove it and I wasn't aware of it when I cloned the repo, because the first thing I did was to remove all css and all comments and changing the import afterwards didn't make any difference because I used an async property. Anyway, I will close the issue now.

Though I suggest commenting the vulcanized to avoid future confusion.

Thanks

samccone commented 8 years ago

@ebidel @LoveAndHappiness going to reopen this for a moment, would love to hear @arthurvr's and @sindresorhus's thoughts around this because overall we try and avoid "compiled" code for the examples for the reasons you so elegantly pointed out @LoveAndHappiness

But I believe that the mission of TodoMVC is to give devs an understanding on how the app works, not necessarily how the app looks like when it is finished.

You can see this in place with our vanilla JS react examples and how we are using the browser version of the JSX compiler, which you would never do in the real world

ebidel commented 8 years ago

The goal of TodoMVC is meant to demonstrate app structure and design patterns. Neither is lost in serving a vulcanized version. Refer to https://github.com/tastejs/todomvc/issues/1490#issuecomment-150628240. The app structure is no different when using the vulcanized file. The key point is that the unminified source is checked in alongside the vulcanized bundle and it's easy to switch between for testing.

arthurvr commented 8 years ago

I was against it from the beginning and don't see why this app needs to be different than all other build systems, which we also disallow. It adds complexity and doesn't add any value when comparing frameworks. It only makes it (even if it's just a little bit) more difficult to dig into the source code and the performance improvements don't really matter, we even warn people about that right in the readme:

Also, please keep in mind that TodoMVC is not the perfect way to compare the size of different frameworks. We intentionally use the unminified versions to make reading the source code easier.

passy commented 8 years ago

We deliberate don't have strict rules for or against compiled code, because there's often a balance to strike between idiomatic use and "editability". Both are core goals with TodoMVC but I think it's best to make this decision on a case by case basis. As @arthurvr pointed out, loading times is not something we optimize for, but in Polymer's case it's more about the HTML imports than that.

ebidel commented 8 years ago

Agree with Pascal. Polymer is unique here in that it requires polyfills. No other example is in the same boat.

Loading is super fast in Chrome because it's native. Other browsers need vulcanized help. The problem is that developers see differences when testing in different browsers and end up drawing incorrect conclusions from it.

On Sat, Oct 24, 2015, 3:54 AM Pascal Hartig notifications@github.com wrote:

We deliberate don't have strict rules for or against compiled code, because there's often a balance to strike between idiomatic use and "editability". Both are core goals with TodoMVC but I think it's best to make this decision on a case by case basis. As @arthurvr https://github.com/arthurvr pointed out, loading times is not something we optimize for, but in Polymer's case it's more about the HTML imports than that.

— Reply to this email directly or view it on GitHub https://github.com/tastejs/todomvc/issues/1490#issuecomment-150784038.

LoveAndHappiness commented 8 years ago

I believe there has been plenty of opportunity to bring forward all arguments, so that the issue can be dealt with or closed, because we can expect that no additional information will be introduced.

Polymer has a build step in it that is only retroactively alterable by going through the comments.

There is consensus that this build step adds additonal complexity and also implies that developers also have to know how to implement the build step to reach the demonstrated application, when rebuilding the application.

@arthurvr rightfully suggested that any other framework could also implement build steps, best practices and design patterns and thus obfuscate the end result. Thus making it necessary to scan through comments and readmes to understand how the application has been created.

@ebidel claims that the reason for adding complexity through build steps is that developers would draw incorrect conclusions from testing with different browsers. An argument based rather on an assumption than a fact.

So the next thing to happen is to decide:

Does the fear of drawing incorrect conclusions by developers as @ebidel claims outweight the need for a consistent and simple TodoMVC application?

In my opinion no, because simplicity outweights perfectionism. Simplicity allows for a quicker understanding and faster adoption.

I am rather new to github, so I am not sure how things are decided here. If it is democratic I pledge for an alteration of the code.

samccone commented 8 years ago

I am in agreement with you @LoveAndHappiness

Polymer having a build step makes it an outlier (of examples that can be run without a compile step {not compile-to-js}) in our examples, while yes it is not representative of a performant polymer app, the same can be said about almost all of the other examples, they are using techniques that are not the fastest since we are optimizing for simplicity and not performance.

:+1: to drop the build step from polymer and go back to the raw version.

Thanks for the thought out comment @LoveAndHappiness

As a team, we are thinking around ways to give devs an option to build and deliver an "optimized" version but that conversation can happen in another issue.

ebidel commented 8 years ago

Please don't drop it. Polymer is unique to every example in TodoMVC because it requires polyfills. Am I right that no other example does?

An argument based rather on an assumption than a fact.

@LoveAndHappiness it's far from an assumption. This is from years of working on this stuff and talking with developers. People are very quick to form FUD and draw incorrect conclusions about libraries without knowing detail or context. Added to this is the unfortunate fact that TodoMVC has become a "standard" for benchmarking libraries/frameworks. But that's where we've ended up.

build step adds additional complexity and also implies that developers also have to know how to implement the build step to reach the demonstrated application

I'd argue that anyone interested in the Polymer example should also be interested in making fast apps. For the short term, this requires them to need vulcanize. TodoMVC is a great way to discover TheRightWay to build an app in a lib/framework. Not incorporating a best practice into a beginner's example, is IMO, irresponsible. The Polymer team wants to start developers down the right path. This is really important and something all samples in TodoMVC should really adopt. People turn to these examples for patterns and best practices.

Thus making it necessary to scan through comments and READMEs to understand how the application has been created.

Given that people are learning from these examples, anyone checking out code will be scanning though readmes and looking at comments. I don't think that should be a concern.

My proposal still stands: If there's a way for todomvc.com to serve the built version and devs get the unbuilt version on checkout, that would alleviate most of my concern. We could switch the default to elements.html instead of elements.vulcanized.html.

addyosmani commented 8 years ago

As a project, we've increasingly seen TodoMVC used for performance benchmarking, despite not being originally designed for this purpose. Our primary goals have always been ease of code readability and learning, as reflected by comments from core above. That said, the onus ultimately falls on us to craft a very good solution for the perf use-case if we want to continue to be used for this purpose.

Safari/WebKit, Chrome, and FilamentGroup are all folks that have used us for this over the last 2-3 years and I think we have value to offer there. Right now, almost none of our apps are prod ready at all.

I see a few possible paths moving forward (there are likely many more):

Ultimately, if we go for any of these options I think including a production version needs to be completely optional, however if an app/framework author does pursue this option we need some level of commitment that the prod versions are going to be kept up to date with the main implementations.

/ducks