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

Remove ../'s from bower_components paths #236

Merged
merged 1 commit into from
Dec 19, 2014
Merged

Remove ../'s from bower_components paths #236

merged 1 commit into from
Dec 19, 2014

Conversation

silvenon
Copy link
Member

This is my proposition for the bower_components mess. Instead of only fixing that, I went further and removed bower_components from URLs completely, because to me they are kind of verbose and potential issues with name conflicts seem unlikely enough.

So instead of:

<script src="../bower_components/modernizr/modernizr.js"></script>

we would have:

<script src="modernizr/modernizr.js"></script>

Same goes for Sass. No more ../../../../ sillyness.

Fixes #150.

@silvenon
Copy link
Member Author

The verbosity of wiredep commands is increasing, but if we get #235 approved, we can apply the wiredep command with same options for both HTML and Sass.

@silvenon
Copy link
Member Author

Also, this adds some additional time to the html task and probably styles task. But I would sacrifice speed for clarity in this case.

@austinpray
Copy link

I do not see any benefit to this whatsoever. If anything more verbosity is better, especially for first-time users. This is a generator, it should be as succinct and flexible as possible.

These paths are being managed by a robot anyway. Nobody is manually manipulating these.

@silvenon
Copy link
Member Author

The logic of the ../bower_components path in HTML files is terrible. We're using the fact that ../ = / when we're already in the root, just so we don't trip up gulp-useref. We've had numerous issues about this subject and if we have to explain something many times, it's not a good design.

Also, when I'm debugging what wiredep injected in my file, I already know it's from Bower, so I don't need that ../bower_components prefix, it's clutter to me. The robot is managing them, but a human is reading them from time to time.

@Munter
Copy link

Munter commented Nov 30, 2014

I do not agree with this change. All urls should always be valid file references. If you part with this, you add complexity for the reader because they have to make a mental leap each time they need to resolve an url. Verbosity does not equal complexity. Often times the opposite is true. And this change takes the generator more in the direction of setting you up with a static page generator than an actual webpage.

That said, the /../ url is an abonimation that should die a swift death. The solution is not to break all urls though. Instead bower_components should reside inside the web root. This would move the generator closer to actually generating a website that can be used even without the build step, thus further decreasing complexity. This is exactly what I did in generator-greenfield.

@addyosmani
Copy link
Member

I'm +1 with @Munter about this adding complexity, but sympathise with the desire to simplify this setup. It's hard because bower_components absolutely does = clutter.

We opted for similar file path patterns to what you're suggesting for Polymer projects and whilst it makes sense for an experienced developer, you'll find it introduces cognitive overhead for beginners who just want to point to clear paths.

@silvenon
Copy link
Member Author

Ok, I agree with you guys. I actually mainly want to remove the ../ abomination :D How about we then convert:

<script src="../bower_components/modernizr/modernizr.js"></script>

to:

<script src="bower_components/modernizr/modernizr.js"></script>

I don't think we have to move bower_components to app, though.

@silvenon
Copy link
Member Author

silvenon commented Dec 7, 2014

Ok, now we have an inconsistency:

HTML:

<script src="/bower_components/modernizr/modernizr.js"></script>

Sass:

@import "bower_components/bootstrap-sass-official/assets/stylesheets/_bootstrap.scss";

/bower_components path crashes Sass, maybe load paths don't work that way. Do I remove / from HTML path as well?

@silvenon
Copy link
Member Author

Ok, removed the inconsistency. Both now start with bower_components/.

This is ready for merging, if you agree, @eddiemonge @sindresorhus @Munter @addyosmani.

@eddiemonge
Copy link
Member

seems fine to me 👍

@silvenon
Copy link
Member Author

Rebased.

@silvenon silvenon changed the title Remove bower_components path from asset URLs Remove ../'s from bower_components paths Dec 15, 2014
@silvenon
Copy link
Member Author

By the way, this is now in sync with how generator-webapp does it.

@@ -217,6 +217,7 @@ module.exports = yeoman.generators.Base.extend({
bowerJson: bowerJson,
directory: 'bower_components',
exclude: ['bootstrap-sass', 'bootstrap.js'],
ignorePath: /^(\.\.\/)+/,
Copy link
Member

Choose a reason for hiding this comment

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

ignorePath: /^\.\.\//,

Choose a reason for hiding this comment

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

If just one ../ is being replaced, a string: "../" also works.

@sindresorhus
Copy link
Member

I'm fine with this, but can you document the behaviour in the readme as it's a bit magic?

@silvenon
Copy link
Member Author

As this is more philosophically-oriented, I would rather put it under docs/, not in the main readme.

@sindresorhus
Copy link
Member

Yeah, sure. Just needs to be written down somewhere.

@silvenon
Copy link
Member Author

Added the notes.

sindresorhus added a commit that referenced this pull request Dec 19, 2014
Remove ../'s from bower_components paths
@sindresorhus sindresorhus merged commit c32e765 into yeoman:master Dec 19, 2014
@sindresorhus
Copy link
Member

Lgtm. Nice work @silvenon! :)

5fa79994-b5ee-11e3-9a4d-b19557d7480b

@silvenon silvenon deleted the bower branch December 19, 2014 02:32
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.

Out of date comment in the connect task?
7 participants