Skip to content

Commit

Permalink
postcss: Fix import error handling
Browse files Browse the repository at this point in the history
Note that we will now fail if `inlineImports` is enabled and we cannot resolve an import.

You can work around this by either:

* Use url imports or imports with media queries.
* Set `skipInlineImportsNotFound=true` in the options

Also get the argument order in the different NewFileError* funcs in line.

Fixes #9895
  • Loading branch information
bep committed May 15, 2022
1 parent c2fa0a3 commit 4b189d8
Show file tree
Hide file tree
Showing 20 changed files with 157 additions and 41 deletions.
48 changes: 40 additions & 8 deletions common/herrors/file_error.go
Expand Up @@ -142,7 +142,7 @@ func (e *fileError) Unwrap() error {
// NewFileError creates a new FileError that wraps err.
// The value for name should identify the file, the best
// being the full filename to the file on disk.
func NewFileError(name string, err error) FileError {
func NewFileError(err error, name string) FileError {
// Filetype is used to determine the Chroma lexer to use.
fileType, pos := extractFileTypePos(err)
pos.Filename = name
Expand All @@ -155,7 +155,7 @@ func NewFileError(name string, err error) FileError {
}

// NewFileErrorFromPos will use the filename and line number from pos to create a new FileError, wrapping err.
func NewFileErrorFromPos(pos text.Position, err error) FileError {
func NewFileErrorFromPos(err error, pos text.Position) FileError {
// Filetype is used to determine the Chroma lexer to use.
fileType, _ := extractFileTypePos(err)
if fileType == "" {
Expand All @@ -165,20 +165,52 @@ func NewFileErrorFromPos(pos text.Position, err error) FileError {

}

func NewFileErrorFromFileInPos(err error, pos text.Position, fs afero.Fs, linematcher LineMatcherFn) FileError {
if err == nil {
panic("err is nil")
}
f, realFilename, err2 := openFile(pos.Filename, fs)
if err2 != nil {
return NewFileErrorFromPos(err, pos)
}
pos.Filename = realFilename
defer f.Close()
return NewFileErrorFromPos(err, pos).UpdateContent(f, linematcher)
}

// NewFileErrorFromFile is a convenience method to create a new FileError from a file.
func NewFileErrorFromFile(err error, filename, realFilename string, fs afero.Fs, linematcher LineMatcherFn) FileError {
func NewFileErrorFromFile(err error, filename string, fs afero.Fs, linematcher LineMatcherFn) FileError {
if err == nil {
panic("err is nil")
}
if linematcher == nil {
linematcher = SimpleLineMatcher
f, realFilename, err2 := openFile(filename, fs)
if err2 != nil {
return NewFileError(err, realFilename)
}
defer f.Close()
return NewFileError(err, realFilename).UpdateContent(f, linematcher)
}

func openFile(filename string, fs afero.Fs) (afero.File, string, error) {
realFilename := filename

// We want the most specific filename possible in the error message.
fi, err2 := fs.Stat(filename)
if err2 == nil {
if s, ok := fi.(interface {
Filename() string
}); ok {
realFilename = s.Filename()
}

}

f, err2 := fs.Open(filename)
if err2 != nil {
return NewFileError(realFilename, err)
return nil, realFilename, err2
}
defer f.Close()
return NewFileError(realFilename, err).UpdateContent(f, linematcher)

return f, realFilename, nil
}

// Cause returns the underlying error or itself if it does not implement Unwrap.
Expand Down
4 changes: 2 additions & 2 deletions common/herrors/file_error_test.go
Expand Up @@ -30,7 +30,7 @@ func TestNewFileError(t *testing.T) {

c := qt.New(t)

fe := NewFileError("foo.html", errors.New("bar"))
fe := NewFileError(errors.New("bar"), "foo.html")
c.Assert(fe.Error(), qt.Equals, `"foo.html:1:1": bar`)

lines := ""
Expand Down Expand Up @@ -70,7 +70,7 @@ func TestNewFileErrorExtractFromMessage(t *testing.T) {
{errors.New(`execute of template failed: template: index.html:2:5: executing "index.html" at <partial "foo.html" .>: error calling partial: "/layouts/partials/foo.html:3:6": execute of template failed: template: partials/foo.html:3:6: executing "partials/foo.html" at <.ThisDoesNotExist>: can't evaluate field ThisDoesNotExist in type *hugolib.pageStat`), 0, 2, 5},
} {

got := NewFileError("test.txt", test.in)
got := NewFileError(test.in, "test.txt")

errMsg := qt.Commentf("[%d][%T]", i, got)

Expand Down
2 changes: 1 addition & 1 deletion config/configLoader.go
Expand Up @@ -59,7 +59,7 @@ func FromConfigString(config, configType string) (Provider, error) {
func FromFile(fs afero.Fs, filename string) (Provider, error) {
m, err := loadConfigFromFile(fs, filename)
if err != nil {
return nil, herrors.NewFileErrorFromFile(err, filename, filename, fs, nil)
return nil, herrors.NewFileErrorFromFile(err, filename, fs, nil)
}
return NewFrom(m), nil
}
Expand Down
4 changes: 4 additions & 0 deletions docs/content/en/hugo-pipes/postcss.md
Expand Up @@ -44,6 +44,10 @@ URL imports (e.g. `@import url('https://fonts.googleapis.com/css?family=Open+San
Note that this import routine does not care about the CSS spec, so you can have @import anywhere in the file.
Hugo will look for imports relative to the module mount and will respect theme overrides.

skipInlineImportsNotFound [bool] {{< new-in "0.99.0" >}}

Before Hugo 0.99.0 when `inlineImports` was enabled and we failed to resolve an import, we logged it as a warning. We now fail the build. If you have regular CSS imports in your CSS that you want to preserve, you can either use imports with URL or media queries (Hugo does not try to resolve those) or set `skipInlineImportsNotFound` to true.

_If no configuration file is used:_

use [string]
Expand Down
11 changes: 11 additions & 0 deletions hugofs/fileinfo.go
Expand Up @@ -139,6 +139,17 @@ type fileInfoMeta struct {
m *FileMeta
}

type filenameProvider interface {
Filename() string
}

var _ filenameProvider = (*fileInfoMeta)(nil)

// Filename returns the full filename.
func (fi *fileInfoMeta) Filename() string {
return fi.m.Filename
}

// Name returns the file's name. Note that we follow symlinks,
// if supported by the file system, and the Name given here will be the
// name of the symlink, which is what Hugo needs in all situations.
Expand Down
2 changes: 1 addition & 1 deletion hugolib/config.go
Expand Up @@ -511,5 +511,5 @@ func (configLoader) loadSiteConfig(cfg config.Provider) (scfg SiteConfig, err er
}

func (l configLoader) wrapFileError(err error, filename string) error {
return herrors.NewFileErrorFromFile(err, filename, filename, l.Fs, nil)
return herrors.NewFileErrorFromFile(err, filename, l.Fs, nil)
}
2 changes: 1 addition & 1 deletion hugolib/hugo_sites.go
Expand Up @@ -968,7 +968,7 @@ func (h *HugoSites) errWithFileContext(err error, f source.File) error {
}
realFilename := fim.Meta().Filename

return herrors.NewFileErrorFromFile(err, realFilename, realFilename, h.SourceSpec.Fs.Source, nil)
return herrors.NewFileErrorFromFile(err, realFilename, h.SourceSpec.Fs.Source, nil)

}

Expand Down
4 changes: 2 additions & 2 deletions hugolib/page.go
Expand Up @@ -588,7 +588,7 @@ func (p *pageState) wrapError(err error) error {
}
}

return herrors.NewFileErrorFromFile(err, filename, filename, p.s.SourceSpec.Fs.Source, herrors.NopLineMatcher)
return herrors.NewFileErrorFromFile(err, filename, p.s.SourceSpec.Fs.Source, herrors.NopLineMatcher)

}

Expand Down Expand Up @@ -788,7 +788,7 @@ func (p *pageState) outputFormat() (f output.Format) {

func (p *pageState) parseError(err error, input []byte, offset int) error {
pos := p.posFromInput(input, offset)
return herrors.NewFileError(p.File().Filename(), err).UpdatePosition(pos)
return herrors.NewFileError(err, p.File().Filename()).UpdatePosition(pos)
}

func (p *pageState) pathOrTitle() string {
Expand Down
4 changes: 2 additions & 2 deletions hugolib/shortcode.go
Expand Up @@ -298,7 +298,7 @@ func renderShortcode(
var err error
tmpl, err = s.TextTmpl().Parse(templName, templStr)
if err != nil {
fe := herrors.NewFileError(p.File().Filename(), err)
fe := herrors.NewFileError(err, p.File().Filename())
pos := fe.Position()
pos.LineNumber += p.posOffset(sc.pos).LineNumber
fe = fe.UpdatePosition(pos)
Expand Down Expand Up @@ -391,7 +391,7 @@ func renderShortcode(
result, err := renderShortcodeWithPage(s.Tmpl(), tmpl, data)

if err != nil && sc.isInline {
fe := herrors.NewFileError(p.File().Filename(), err)
fe := herrors.NewFileError(err, p.File().Filename())
pos := fe.Position()
pos.LineNumber += p.posOffset(sc.pos).LineNumber
fe = fe.UpdatePosition(pos)
Expand Down
2 changes: 1 addition & 1 deletion langs/i18n/translationProvider.go
Expand Up @@ -138,6 +138,6 @@ func errWithFileContext(inerr error, r source.File) error {
}
defer f.Close()

return herrors.NewFileError(realFilename, inerr).UpdateContent(f, nil)
return herrors.NewFileError(inerr, realFilename).UpdateContent(f, nil)

}
2 changes: 1 addition & 1 deletion markup/goldmark/codeblocks/render.go
Expand Up @@ -130,7 +130,7 @@ func (r *htmlRenderer) renderCodeBlock(w util.BufWriter, src []byte, node ast.No
ctx.AddIdentity(cr)

if err != nil {
return ast.WalkContinue, herrors.NewFileErrorFromPos(cbctx.createPos(), err)
return ast.WalkContinue, herrors.NewFileErrorFromPos(err, cbctx.createPos())
}

return ast.WalkContinue, nil
Expand Down
2 changes: 1 addition & 1 deletion parser/metadecoders/decoder.go
Expand Up @@ -260,7 +260,7 @@ func (d Decoder) unmarshalORG(data []byte, v any) error {
}

func toFileError(f Format, data []byte, err error) error {
return herrors.NewFileError(fmt.Sprintf("_stream.%s", f), err).UpdateContent(bytes.NewReader(data), nil)
return herrors.NewFileError(err, fmt.Sprintf("_stream.%s", f)).UpdateContent(bytes.NewReader(data), nil)
}

// stringifyMapKeys recurses into in and changes all instances of
Expand Down
2 changes: 1 addition & 1 deletion resources/resource_transformers/js/build.go
Expand Up @@ -165,7 +165,7 @@ func (t *buildTransformation) Transform(ctx *resources.ResourceTransformationCtx

if err == nil {
fe := herrors.
NewFileError(path, errors.New(errorMessage)).
NewFileError(errors.New(errorMessage), path).
UpdatePosition(text.Position{Offset: -1, LineNumber: loc.Line, ColumnNumber: loc.Column}).
UpdateContent(f, nil)

Expand Down
48 changes: 47 additions & 1 deletion resources/resource_transformers/postcss/integration_test.go
Expand Up @@ -48,7 +48,7 @@ class-in-b {
@tailwind base;
@tailwind components;
@tailwind utilities;
@import "components/all.css";
@import "components/all.css";
h1 {
@apply text-2xl font-bold;
}
Expand Down Expand Up @@ -140,3 +140,49 @@ func TestTransformPostCSSError(t *testing.T) {
c.Assert(err.Error(), qt.Contains, "a.css:4:2")

}

// #9895
func TestTransformPostCSSImportError(t *testing.T) {
if !htesting.IsCI() {
t.Skip("Skip long running test when running locally")
}

c := qt.New(t)

s, err := hugolib.NewIntegrationTestBuilder(
hugolib.IntegrationTestConfig{
T: c,
NeedsOsFS: true,
NeedsNpmInstall: true,
LogLevel: jww.LevelInfo,
TxtarString: strings.ReplaceAll(postCSSIntegrationTestFiles, `@import "components/all.css";`, `@import "components/doesnotexist.css";`),
}).BuildE()

s.AssertIsFileError(err)
c.Assert(err.Error(), qt.Contains, "styles.css:4:3")
c.Assert(err.Error(), qt.Contains, filepath.FromSlash(`failed to resolve CSS @import "css/components/doesnotexist.css"`))

}

func TestTransformPostCSSImporSkipInlineImportsNotFound(t *testing.T) {
if !htesting.IsCI() {
t.Skip("Skip long running test when running locally")
}

c := qt.New(t)

files := strings.ReplaceAll(postCSSIntegrationTestFiles, `@import "components/all.css";`, `@import "components/doesnotexist.css";`)
files = strings.ReplaceAll(files, `{{ $options := dict "inlineImports" true }}`, `{{ $options := dict "inlineImports" true "skipInlineImportsNotFound" true }}`)

s := hugolib.NewIntegrationTestBuilder(
hugolib.IntegrationTestConfig{
T: c,
NeedsOsFS: true,
NeedsNpmInstall: true,
LogLevel: jww.LevelInfo,
TxtarString: files,
}).Build()

s.AssertFileContent("public/css/styles.css", filepath.FromSlash(`@import "components/doesnotexist.css";`))

}
42 changes: 32 additions & 10 deletions resources/resource_transformers/postcss/postcss.go
Expand Up @@ -28,6 +28,7 @@ import (

"github.com/gohugoio/hugo/common/collections"
"github.com/gohugoio/hugo/common/hexec"
"github.com/gohugoio/hugo/common/text"
"github.com/gohugoio/hugo/hugofs"

"github.com/gohugoio/hugo/common/hugo"
Expand All @@ -49,9 +50,10 @@ import (

const importIdentifier = "@import"

var cssSyntaxErrorRe = regexp.MustCompile(`> (\d+) \|`)

var shouldImportRe = regexp.MustCompile(`^@import ["'].*["'];?\s*(/\*.*\*/)?$`)
var (
cssSyntaxErrorRe = regexp.MustCompile(`> (\d+) \|`)
shouldImportRe = regexp.MustCompile(`^@import ["'].*["'];?\s*(/\*.*\*/)?$`)
)

// New creates a new Client with the given specification.
func New(rs *resources.Spec) *Client {
Expand Down Expand Up @@ -100,6 +102,12 @@ type Options struct {
// so you can have @import anywhere in the file.
InlineImports bool

// When InlineImports is enabled, we fail the build if an import cannot be resolved.
// You can enable this to allow the build to continue and leave the import statement in place.
// Note that the inline importer does not process url location or imports with media queries,
// so those will be left as-is even without enabling this option.
SkipInlineImportsNotFound bool

// Options for when not using a config file
Use string // List of postcss plugins to use
Parser string // Custom postcss parser
Expand Down Expand Up @@ -204,6 +212,7 @@ func (t *postcssTransformation) Transform(ctx *resources.ResourceTransformationC
imp := newImportResolver(
ctx.From,
ctx.InPath,
t.options,
t.rs.Assets.Fs, t.rs.Logger,
)

Expand Down Expand Up @@ -239,19 +248,21 @@ type fileOffset struct {
type importResolver struct {
r io.Reader
inPath string
opts Options

contentSeen map[string]bool
linemap map[int]fileOffset
fs afero.Fs
logger loggers.Logger
}

func newImportResolver(r io.Reader, inPath string, fs afero.Fs, logger loggers.Logger) *importResolver {
func newImportResolver(r io.Reader, inPath string, opts Options, fs afero.Fs, logger loggers.Logger) *importResolver {
return &importResolver{
r: r,
inPath: inPath,
fs: fs, logger: logger,
linemap: make(map[int]fileOffset), contentSeen: make(map[string]bool),
opts: opts,
}
}

Expand Down Expand Up @@ -282,21 +293,32 @@ func (imp *importResolver) importRecursive(
i := 0
for offset, line := range lines {
i++
line = strings.TrimSpace(line)
lineTrimmed := strings.TrimSpace(line)
column := strings.Index(line, lineTrimmed)
line = lineTrimmed

if !imp.shouldImport(line) {
trackLine(i, offset, line)
} else {
i--
path := strings.Trim(strings.TrimPrefix(line, importIdentifier), " \"';")
filename := filepath.Join(basePath, path)
importContent, hash := imp.contentHash(filename)

if importContent == nil {
trackLine(i, offset, "ERROR")
imp.logger.Warnf("postcss: Failed to resolve CSS @import in %q for path %q", inPath, filename)
continue
if imp.opts.SkipInlineImportsNotFound {
trackLine(i, offset, line)
continue
}
pos := text.Position{
Filename: inPath,
LineNumber: offset + 1,
ColumnNumber: column + 1,
}
return 0, "", herrors.NewFileErrorFromFileInPos(fmt.Errorf("failed to resolve CSS @import %q", filename), pos, imp.fs, nil)
}

i--

if imp.contentSeen[hash] {
i++
// Just replace the line with an empty string.
Expand Down Expand Up @@ -399,7 +421,7 @@ func (imp *importResolver) toFileError(output string) error {
}
defer f.Close()

ferr := herrors.NewFileError(realFilename, inErr)
ferr := herrors.NewFileError(inErr, realFilename)
pos := ferr.Position()
pos.LineNumber = file.Offset + 1
return ferr.UpdatePosition(pos).UpdateContent(f, nil)
Expand Down

0 comments on commit 4b189d8

Please sign in to comment.