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

Fixing import by reference #2729

Merged
merged 2 commits into from
Dec 7, 2015
Merged

Fixing import by reference #2729

merged 2 commits into from
Dec 7, 2015

Conversation

meri added 2 commits November 20, 2015 15:10
- refactored how import reference works
- refactored to-css-visitor (this is side product, it was getting
  complicated)
- fixes issues less#1851, less#1896, less#1878, less#2716, less#1968, less#2162 (same as less#1896)
@matthew-dean
Copy link
Member

Wow, nice work!

@seven-phases-max
Copy link
Member

👍

@SomMeri
Copy link
Member Author

SomMeri commented Nov 27, 2015

@matthew-dean @seven-phases-max Thank you. This took more then one attempt to get it fully (I hope) working.

If nobody objects, I am going to merge it in sometimes next week.

@SomMeri
Copy link
Member Author

SomMeri commented Nov 27, 2015

This comment describes how changes in pull requests are supposed to work.

---- Relevant less.js workflow before changes (this is visible in transform-tree.js):

Phase 1: eval - solves imports, mixins calls and detached rulesets calls

  • imports are replaced by parsed files
  • mixin calls are replaced by content of mixin definitions
  • detached rulesets calls are replaced by contents called rulesets

Phase 2: JoinSelectorVisitor - joins selectors (paths) in nested rulesets with selectors in their parents

Phase 3: ExtendVisitor - process all extends

Phase 4: ToCSSVisitor - each visitX function in ToCSSVisitor is responsible for returning its own css equivalent.

  • removes nested rulesets from their parents
  • removes what should be invisible

The end result should be valid css tree which is then converted into text.

-------- New properties:
node.js has two new properties:

  • nodeVisible - boolean returned by isVisible function
  • visibilityBlocks - counter which can be raised by one or lowered by one through add and remove functions

Visibility blocks are placed on nodes imported by reference. Blocks are NOT placed on their childs, only the root of imported tree has visibility block. Adding visibility block on node means: "this node is root of tree that replaced imported by reference" or alternatively "this node came from one more import by reference then its parent".

Property isVisible can return one of three values:

  • true - the node is visible
  • undefined or null - undefined visibility, assumed to be the same as its parents visibility
  • false - the node must not be visible (not used in practice)

Note: isVisible matters only for selector up until we hit the phase 4. - ToCSSVisitor.

-------- New visibility evaluation
Phase 1: eval changes:

  • Import by reference adds visibility block to roots of whatever is imported.
  • Mixin call with visibility block on it must add a visibility block to everything it is replacing itself with. The block would be lost otherwise.
  • Detached ruleset with visibility block on it adds a visibility block to everything it is replacing itself with. The block would be lost otherwise.

Phase 2: JoinSelectorVisitor (e.g. selectors in nested rulesets) - no change

New Phase 2.5: Starting from root of ast, everything up to first visibility block is marked as "visible". Nodes with visibility block and their child are left unmodified.

Important outcome: each selector and extends now knows whether it came from visible part of the tree or or from something hidden being import by reference.

Phase 3: ExtendVisitor

  • Each newly created selector gets the same visibility as the extend who created it. Visible extends produce visible selectors and invisible extends produce invisible selectors. Important outcome: all selectors are extended and know whether they should be visible or not.

Phase 4: ToCSSVisitor - In addition to generating its own css equivalent, each visitX function is now also responsible for setting replacements isVisible value to either:

  • true - show me even if parent is hidden behind visibility block (e.g. I or something inside me came from extend),
  • undefined or null - show me only if the parent is not hidden behind visibility block
  • Note: if the node thinks it should not be shown at all, then it removes itself.

New visitX methods logic:

  • Each simple node (such as rule, comment, anonymous, import etc) first checks whether it has visibility block. If the block is set, the node removes itself from the tree. Visibility property remains undefined.
  • Rulesets: each ruleset removes all invisible selectors. If the ruleset ends up without visible selectors, then it is removed. If some selector stays, visibility is set on true.
  • Directives and media: directive and media with visibility blocks on them still can be shown if something inside them was extended. Therefore they need to compile their bodies first to see what shows up. E.g. each directive with visibility block:
    • Compiles body.
    • Removes everything that does not have isVisible set on true explictly
    • If something stays in after that, directive visibility is set on true. Otherwise directive is removed.

@matthew-dean
Copy link
Member

Just curious, does this alter the structure of the AST in any significant way? I was going to start work on a postcss-less plugin that would convert the Less AST into PostCSS nodes so that you could use less in a PostCSS pipeline.

@matthew-dean
Copy link
Member

Also, I know that @lukeapage is working on refactoring the Less lib into ES6 + Babel, so with all that refactoring, might be good to coordinate.

@matthew-dean
Copy link
Member

One more thing, this might be a good opportunity to clean up our PRs at the same time and either merge or reject.

@SomMeri
Copy link
Member Author

SomMeri commented Nov 28, 2015

@matthew-dean It does not alter structure at all, just adds two new properties to Node object.

@matthew-dean
Copy link
Member

@SomMeri 👍

SomMeri added a commit that referenced this pull request Dec 7, 2015
Fixing import by reference
@SomMeri SomMeri merged commit baf45ea into less:master Dec 7, 2015
@SomMeri
Copy link
Member Author

SomMeri commented Dec 7, 2015

I merged the pull request in.

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.

3 participants