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

Cannot access site config from specs #1236

Closed
agross opened this issue Oct 23, 2017 · 10 comments
Closed

Cannot access site config from specs #1236

agross opened this issue Oct 23, 2017 · 10 comments

Comments

@agross
Copy link
Contributor

agross commented Oct 23, 2017

I need to access the site config in some of my nanoc extensions. These extensions cannot access @context directly, but use the compilation_context attached to the CompilationItemView. The feature breaks when updating from 4.8.9 to 4.8.10, but only in specs. At runtime the site config is accessible.

Steps to reproduce

The following test succeeds with nanoc 4.8.9 and fails with 4.8.10.

describe 'test' do
  include(Nanoc::Spec::HelperHelper)

  subject { ctx.items['/index.html'] }

  before do
    ctx.create_item('item', {}, '/index.html')
       .create_rep(subject, subject.identifier.to_s)
  end

  it 'has config' do
    expect(subject.instance_variable_get(:@context).compilation_context.site).to respond_to(:config)
  end
end
@denisdefreyne
Copy link
Member

Hi,

First of all, please note that this touches internal APIs, which are not supported and can (and will) break from time to time.

Would it be possible for your code to access @config directly instead?

@agross
Copy link
Contributor Author

agross commented Oct 23, 2017

Hi,

Yes, I know that I am touching internal types. 4.8.10 breaks other APIs (e.g. ViewContext -> ViewContextForCompilation) that I already fixed in my project. (I assumed that everything outside of Nanoc::Int is public, but I see this class is not documented -- so I guess it's meant to be internal.)

As we discussed earlier, I'm not a fan of polluting global objects (even if it is just the CompilationContext), so most methods are attached to the classes that they make sense for. In this instance, I need to construct absolute URLs for a given item. Instead of absolute_url_for(item) I can write item.url(absolute: true) which then needs to access the site's config to get the base URL.

As it stands the issue only arises in tests, but from the diff I couldn't find the location where it broke, otherwise I would have submitted a PR.

To answer your question, I could go through all my layouts and content files and replace #url with a helper that you would prefer. But I have to admit that I would be in favor of a solution that restores the functionality of accessing the site's config in test contexts.

@denisdefreyne
Copy link
Member

Your approach of modifying internal classes is problematic for two reasons:

  • Your code might break in every patch release of Nanoc.

  • The behavior that you are relying on (view context, compilation_context, …) can have unintended and harmful consequences. In your particular case, accessing the configuration through the compilation context bypasses the dependency tracking mechanism.

I strongly recommend not modifying existing classes. In your case, prefer absolute_url_for(item) over item.url(absolute: true). I understand that this not the style that you want, but it is the only one that Nanoc supports.

I assumed that everything outside of Nanoc::Int is public, but I see this class is not documented -- so I guess it's meant to be internal.

Everything inside Nanoc::Int is internal, but there are also classes outside of Nanoc::Int that are private. They‘ll be marked with @api private — but even if they aren’t, it’s a good bet to assume that they are also private. The only public API is documented on the Variables page on the Nanoc web site.

@agross
Copy link
Contributor Author

agross commented Oct 23, 2017

OK, I'm going to investigate this. The first spec should check whether a forwarded item (via frontmatter) yields the correct target URL. The idea is:

  • item forwards to target via frontmatter
  • absolute URL of item is the same as the absolute URL of target
def absolute_url_of(item)
  append = item.path

  if item.attributes[:forward]
    append = @items[item.attributes[:forward]].path
  end
  @config[:base_url] + append
end

describe 'absolute URL' do
  include Nanoc::Spec::HelperHelper

  let(:item) { ctx.items['/index.html'] }
  let(:target) { ctx.items['/somewhere/else.html'] }

  before do
    ctx.create_item('item', { forward: '/somewhere/else.html' }, '/index.html')
       .create_item('target', {}, '/somewhere/else.html')
       .create_rep(item, '/path/to' + item.identifier.to_s)
       .create_rep(target, '/path/to' + target.identifier.to_s)
  end

  it 'yields URL of forwarded item' do
    ctx.instance_exec(item, target) do |item, target|
      url = absolute_url_of(item)
      target = absolute_url_of(target)
      expect(url).to eq(target)
    end
  end
end

Is this the idea behind Nanoc::Spec::HelperHelper and is instance_exec correct?

If so, how would I work around this?

Failures:

  1) absolute URL yields URL of forwarded item
     Failure/Error: append = @items[item.attributes[:forward]].path

     NoMethodError:
       undefined method `path' for <Nanoc::Int::Item identifier="/somewhere/else.html">:Nanoc::Int::Item

@denisdefreyne
Copy link
Member

There’s a few changes that you’ll need:

  1. Put your helper in a module, and then use that as the argument to #describe. Also pass helper: true:

    module AbsoluteURLHelper
      def absolute_url_of(item)
        # …
      end
    end
    
    describe AbsoluteURLHelper, helper: true do
      # …
    end
  2. No need for instance_exec etc. You can call the helper directly:

    expect(helper.absolute_url_of(item)).to eq('/some/specific/place.html')

@denisdefreyne
Copy link
Member

Also, prefer item[:forward] to item.attributes[:forward].

@denisdefreyne
Copy link
Member

denisdefreyne commented Oct 23, 2017

Full spec that works:

module AbsoluteURLHelper
  def absolute_url_of(item)
    append =
      if item[:forward]
        @items[item[:forward]].path
      else
        item.path
      end

    @config[:base_url] + append
  end
end

describe AbsoluteURLHelper, helper: true do
  let(:item) { ctx.items['/index.html'] }
  let(:target) { ctx.items['/somewhere/else.html'] }

  before do
    ctx.config[:base_url] = '/root'

    ctx
      .create_item('item', { forward: '/somewhere/else.html' }, '/index.html')
      .create_item('target', {}, '/somewhere/else.html')
      .create_rep(item, '/path/to' + item.identifier.to_s)
      .create_rep(target, '/path/to' + target.identifier.to_s)
  end

  it 'yields URL of forwarded item' do
    expect(helper.absolute_url_of(item)).to eq('/root/path/to/somewhere/else.html')
    expect(helper.absolute_url_of(target)).to eq('/root/path/to/somewhere/else.html')
  end
end

@denisdefreyne
Copy link
Member

You can leave the include Nanoc::Spec::HelperHelper out, too, by the way. (Already taken care of with helper: true.)

@agross
Copy link
Contributor Author

agross commented Oct 23, 2017

Thanks, that works. I see I got a bug by reassigning target to the URL. I also saw you call the mixin on ctx, so the HelperHelper mixes in the described_class automatically?

You can leave the include Nanoc::Spec::HelperHelper out, too, by the way

Does nanoc hook into rspec for sites? It may work in the nanoc repo itself, but this has spec_helper.rb with c.include(Nanoc::Spec::HelperHelper, helper: true).

@denisdefreyne
Copy link
Member

denisdefreyne commented Oct 24, 2017

If helper: true is passed to a describe block, Nanoc::Spec::HelperHelper is included (mixed in) automatically, which makes ctx and helper available. This will only work with Nanoc’s spec-helper.rb, as you mentioned.

You can also opt to not use helper: true and include Nanoc::Spec::HelperHelper directly.

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

2 participants