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

Polymer example has build step in it #1490

Open
LoveAndHappiness opened this issue Oct 23, 2015 · 17 comments
Open

Polymer example has build step in it #1490

LoveAndHappiness opened this issue Oct 23, 2015 · 17 comments
Labels

Comments

@LoveAndHappiness
Copy link

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
Copy link
Author

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

@samccone
Copy link
Member

cc @addyosmani @ebidel

@samccone
Copy link
Member

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

:)

@ebidel
Copy link
Contributor

ebidel commented Oct 23, 2015

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
Copy link
Contributor

ebidel commented Oct 23, 2015

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

@LoveAndHappiness
Copy link
Author

@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
Copy link
Contributor

ebidel commented Oct 23, 2015

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
Copy link
Author

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
Copy link
Member

@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

@samccone samccone reopened this Oct 23, 2015
@ebidel
Copy link
Contributor

ebidel commented Oct 24, 2015

The goal of TodoMVC is meant to demonstrate app structure and design patterns. Neither is lost in serving a vulcanized version. Refer to #1490 (comment). 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
Copy link
Member

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
Copy link
Member

passy commented Oct 24, 2015

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
Copy link
Contributor

ebidel commented Oct 24, 2015

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 [email protected]
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
#1490 (comment).

@LoveAndHappiness
Copy link
Author

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
Copy link
Member

samccone commented Dec 2, 2015

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.

👍 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
Copy link
Contributor

ebidel commented Dec 2, 2015

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
Copy link
Member

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):

  • The focus of each implementation ultimately remains readability, but each app can have an extra \production directory that includes the build process / optimised source version of the application / final prod index.html. This provides the flexibility needed to have variations in the app source / annotations / tooling but the main examples/<framework>/index.html path still contains the readable version.
  • We create a separate repo called todomvc-perf where (for a much smaller subset of frameworks), we will host performance optimised production versions of apps separate from the main TodoMVC source. The only real downside to this that I see is maintenance overhead and getting people to change the source for an app in two different places. This may get hairy.
  • Projects host their own perf optimised versions with build tooling that we link to. I personally prefer us hosting the optimised source too, but throwing this out as yet another option.

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

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

No branches or pull requests

6 participants