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

Add support for yaml bundles definitions #3136

Closed
wants to merge 4 commits into from
Closed

Add support for yaml bundles definitions #3136

wants to merge 4 commits into from

Conversation

@gwax
Copy link
Contributor

@gwax gwax commented Aug 9, 2018

Pull Request Checklist

  • I’ve read the guidelines for contributing.
  • I updated AUTHORS.txt and CHANGES.txt (if the change is non-trivial) and documentation (if applicable).
  • I tested my changes.

Description

This PR adds support for bundles.yaml or bundles.yml as alternatives to bundles files in themes.

If both are present, the yaml files take precedence. If only bundles is present, behavior is unchanged.

bundles.yaml takes the form:

assets/css/all.css:
  - input1.css
  - input2.css

and would replace bundles file of the form:

assets/css/all.css=input1.css,input2.css
@gwax gwax requested a review from Kwpolska Aug 9, 2018
@gwax gwax force-pushed the yaml_bundles branch from e45c5de to 4862f6f Aug 9, 2018
@gwax
Copy link
Contributor Author

@gwax gwax commented Aug 9, 2018

Baseline is failing because:

  • existing code has a bug wherein assets/js/all-nocdn.js is specified as assets/css/all-nocdn.js in base template; this has been fixed
  • input assets in bundles.yaml files have been sorted alphabetically

I have tested locally against my personal setup

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Aug 9, 2018

👎 from me. What benefits does this have over the old format? I can’t think of any. Since we haven’t migrated config to YAML yet, there is no point in forcing it for simple files like bundles.

existing code has a bug wherein assets/js/all-nocdn.js is specified as assets/css/all-nocdn.js in base template; this has been fixed

Good catch, I fixed this on master.

input assets in bundles.yaml files have been sorted alphabetically

You can’t always do that with JS — for example, if bootstrap.min.js loads before jquery.min.js, it will refuse to work.

@gwax gwax force-pushed the yaml_bundles branch from 4862f6f to b6f0549 Aug 9, 2018
@gwax gwax force-pushed the yaml_bundles branch from b6f0549 to 8b5c938 Aug 9, 2018
@gwax
Copy link
Contributor Author

@gwax gwax commented Aug 9, 2018

Per your point, I have removed the sorting.

@gwax
Copy link
Contributor Author

@gwax gwax commented Aug 9, 2018

The primary benefit over the existing format, and the reason I want this change, is that it allows for bundle inputs to be on separate lines.

During theme development, this helps me in two big ways:

  1. Open base_helper.tmpl and bundles side-by-side to make sure they are in sync with each other
  2. Diff two bundles against each other. This is useful both for parent/child comparison and git change comparison.

In both cases, having to read a long line of entries, especially where I have to visually parse between _, ,, and . can be difficult. I often find myself temporarily inserting line breaks after each comma and then removing them before committing.

I initially considered modifying the existing format to support multiple lines per bundle but supporting yaml seemed much easier and more foolproof.

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Aug 9, 2018

I don’t think using YAML and making it the new default is worth the effort, especially because (a) YAML is not our base dependency, (b) this is minor, “backstage” stuff. I could, however, accept a PR in which bundles.json becomes an alternate accepted format.

@Kwpolska Kwpolska closed this Aug 9, 2018
@Kwpolska Kwpolska deleted the yaml_bundles branch Aug 9, 2018
@gwax
Copy link
Contributor Author

@gwax gwax commented Aug 10, 2018

Instead of changing the file format, I have opened #3137 to change the underlying parser to add additional features while maintaining strict backwards compatibility and without adding new base dependencies.

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

Successfully merging this pull request may close these issues.

None yet

2 participants