Skip to content
This repository was archived by the owner on Jan 11, 2023. It is now read-only.

ignore html files with missing base element #1237

Closed

Conversation

akvadrako
Copy link

I have some html files in my static folder. This caused sapper export to die because it expects all html files to have a <base> element and they do not. This is a simple fix.

@benmccann
Copy link
Member

This seems like a duplicate of #984

@akvadrako
Copy link
Author

I don't think so. This just fixes a bug with export, it doesn't change normal handling of the base element.

@akvadrako
Copy link
Author

It does seem like an alternative solution of #1208 though. That one uses a fake base value to continue crawling. My solution just skips the file if no base is present, which I thought was safer.

Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

oh, I see. thanks for clarifying. I didn't look too closely at first. wouldn't you want the file exported though? otherwise, why have it present in your static directory in the first place?

@akvadrako
Copy link
Author

oh, I see. thanks for clarifying. I didn't look too closely at first. wouldn't you want the file exported though? otherwise, why have it present in your static directory in the first place?

It is exported, it just skips the link crawling.

@benmccann benmccann mentioned this pull request May 29, 2020
@benmccann
Copy link
Member

benmccann commented May 29, 2020

It sounds to me like base will probably be made optional. If that's the case and base is not present on any page where type === 'text/html' then I don't think it'd work very well to skip link crawling on those pages. On the one hand, I understand that this solution would help you immediately. But if we're going to have to come back and change this same section of code I'd rather come up with a long-term solution the first time around especially given that reviewer cycles are so scarce. #1208 is probably more in the right direction for the case where base is optional

@akvadrako
Copy link
Author

In this case it would make most sense to use normal base handling like a browser. So on page /abc/def, base defaults to /abc. Sound good?

@benmccann
Copy link
Member

Yeah, that makes sense to me

@akvadrako
Copy link
Author

Anyway I think it's correct that I close this in favor of 1208 since that one has a test case too.

@akvadrako akvadrako closed this May 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants