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

Block behaviour has changed in extensions between v2.0.0-beta6 and beta11 #2747

Closed
mrsimonemms opened this issue Mar 1, 2017 · 4 comments
Closed

Comments

@mrsimonemms
Copy link

mrsimonemms commented Mar 1, 2017

The behaviour of blocks has changed and can't seem to find this change documented, so wanted to work out how to get the old behaviour.

When extending something with a block, previously I could declare the block and then reuse it infinitely - this was very useful when having a "content" block as each template would just declare an area where to put it.

This is my example:

base.pug

doctype html
html
    body
        div hello
        block content

default.pug

extends ./base.pug

block content
    div header
    .content
        block content
     div footer

index.pug

extends ./default.pug

block content
    b hello world

In 2.0.0-beta6, this would get compiled to:

<!DOCTYPE html>
<html>
  <body>
    <div>header</div>
    <div class="content"><b>hello world</b></div>
    <div>footer</div>
  </body>
</html>

However, in 2.0.0-beta11, this gets compiled to:

<!DOCTYPE html>
<html>
  <body>
    <div class="content"><b>hello world</b></div>
  </body>
</html>
@jeffwilcox
Copy link

+1

@caikan
Copy link

caikan commented Apr 10, 2017

See #2685 .
I have the similar problem:

head.pug

script(src='common-script.js')

layout.pug

html
  head
    block head
      include head.pug
  body

sub-layout.pug

extends layout.pug

block head
  block head
    include head.pug
  script(src='sub-layout-script.js')

page.pug

extends sub-layout.pug

append head
  script(src='page-script.js')

output page.html (version <= 2.0.0-beta7)

<html>
  <head>
    <script src="common-script.js"></script>
    <script src="page-script.js"></script>
    <script src="sub-layout-script.js"></script>
  </head>
  <body></body>
</html>

output page.html (version >= 2.0.0-beta8)

<html>
  <head>
    <script src="common-script.js"></script>
    <script src="page-script.js"></script>
    <script src="sub-layout-script.js"></script>
    <script src="page-script.js"></script>
  </head>
  <body></body>
</html>

@jeffwilcox
Copy link

jeffwilcox commented May 3, 2017

On my end I am able to fix by scoping the overwriting of nodes that happens in the newer version of the linker; in pug-linker, when multiple parents exist, I can limit to a specific filename.

Unfortunately I am not confident committing a fix to this area, especially as the maintainers are finally making great progress with release candidates happening. On my team this does mean we'll either need to rewrite our uses of this nested-block-across-files approach or we will stick with beta7 for the long term until something happens. Happy to try and contribute a fix but weary because of the timeline.

My hack to validate at least that scoping to a file fixes my core scenario:

function flattenParentBlocks(parentBlocks, accumulator, filename) {
  accumulator = accumulator || [];
  parentBlocks.forEach(function (parentBlock) {
    if (parentBlock.parents) {
      flattenParentBlocks(parentBlock.parents, accumulator, parentBlock.filename);
    } else {
      if (filename === undefined || parentBlock.filename === filename) {
        accumulator.push(parentBlock);
      }
    }
  });
  return accumulator;
}

This does break 3 tests, so likely has side effects; but the underlying issue is that with the rewritten/partially refactored declared block support that shipped in beta 8, the root parent's nodes are overwritten to the eventual children nodes, skipping anything in-between.

The reason for the refactoring that the maintainers did was to support scenarios where the same block appears multiple times in a file, a long-time bug. Scenario:

if ajax
  block content
else
  html
    head
      //- etc
    body
      block content

In the multiple-block per file scenario, you legitimately want the nodes of both of these blocks to point at the resolved block nodes, but only because they are in the same file.

Here is a comparison for the underlying work: https://github.com/pugjs/pug/compare/pug@2.0.0-beta7...pug%25402.0.0-beta8

Essentially the issue is when the nodes replacement is done, it ends up pointing too deep in the tree, skipping intermedia nodes for the same blocks in multiple files;

case replace:
  parentBlock.nodes = node.nodes;

Since this overwrites the nodes, starting from the root of the tree for the specific block, the first block will end up pointing to the children, canceling out the intermediate nodes, if any, typically interesting content.

Reading @ForbesLindesay comments in PR #2687, "The only trouble is, I'm not really sure the output was "wrong" to start with. The issue is that append and prepend only really make sense at the top level of a file. By supporting them in nested locations, we end up creating a block as well as appending to the existing block. The simplest solution would be to disallow append/prepend except at the top level. I'm not confident that won't break some people's use cases though.", the underlying concern here really is that there is undefined behavior supporting some concept of nested prepends and other magic.

The scenario that @caikan has is similar to what our team uses it for; our view structure might look like this:

views/
  layout.pug
  settings/
    index.pug
    profile.pug
    layout.pug  

In views/settings/*.pug, files extend the relative local ./layout. Layout defines a block content that adds a sidebar similar to what you'll see on GitHub's settings/profile page for your user account; the left sidebar has an index of the files in that directory. The local layout.pug file then extends ../layout.pug. The fact that the filenames are the same has nothing to do with the bug, however, which is why I appreciate the reproduction of the error that @caikan has - sublayout adjacent to layout.

Thoughts?

@bfricka
Copy link

bfricka commented Jul 18, 2017

This also speaks to a frustrating aspect of this project. No release / change log.

@mrsimonemms mrsimonemms closed this as not planned Won't fix, can't repro, duplicate, stale Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants