Skip to content

Fix filesystem path loading #30

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.

Sign up for GitHub

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

Merged
danielroe merged 3 commits into from
Nov 12, 2024
Merged

Fix filesystem path loading #30

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dee3b5d
Fix filesystem path loading
xpdota Nov 3, 2024
a0881e2
test: add regression test
danielroe Nov 12, 2024
01495ed
test: remove path separators
danielroe Nov 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/beasties-webpack-plugin/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ export default class BeastiesWebpackPlugin extends Beasties {

if (!sheet) {
try {
sheet = await this.readFile(this.compilation, filename)
sheet = await this.readFile(filename)
this.logger.warn(
`Stylesheet "${relativePath}" not found in assets, but a file was located on disk.${
this.options.pruneSource
Expand Down
1 change: 1 addition & 0 deletions packages/beasties-webpack-plugin/test/fixtures/fs-access/dist/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
div.foo{color:red}
12 changes: 12 additions & 0 deletions packages/beasties-webpack-plugin/test/fixtures/fs-access/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<!DOCTYPE html>
<html lang="en">
<head>
<link rel="stylesheet" href="./style.css"/>
<title>Access external stylesheet from FS</title>
</head>
<body>
<div class="foo">
<h1>Access external stylesheet from FS</h1>
</div>
</body>
</html>
Empty file added packages/beasties-webpack-plugin/test/fixtures/fs-access/index.js
View file
Open in desktop
Empty file.
10 changes: 10 additions & 0 deletions packages/beasties-webpack-plugin/test/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,3 +197,13 @@ describe('options', () => {
})
})
})

describe('accessing file system', () => {
it('works', async () => {
const output = await compileToHtml('fs-access', configure, {
path: './dist/',
publicPath: '',
})
Copy link

@coderabbitai coderabbitai bot Nov 12, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

Hard-coded Path Separators Detected

The path handling implementation includes hard-coded path separators which may not resolve correctly across different platforms.

  • Affected files:
    • packages/beasties/test/beasties.test.ts
    • packages/beasties/test/beasties.bench.ts
    • packages/beasties/test/security.test.ts
    • packages/beasties/src/util.ts
    • packages/beasties-webpack-plugin/test/index.test.ts
    • packages/beasties-webpack-plugin/test/standalone.test.ts
    • packages/beasties-webpack-plugin/test/helpers.ts
    • packages/beasties/src/types.ts
    • packages/beasties/src/index.d.ts
    • packages/beasties/src/dom.ts
    • packages/beasties/src/index.ts
    • packages/beasties/src/css.ts
🔗 Analysis chain

Verify path handling implementation

Let's verify that the path handling implementation correctly resolves paths across different platforms.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for platform-specific path handling in the codebase

# Look for path handling logic
echo "Searching for path handling code..."
rg -l "path\.(resolve|join|normalize)" --type ts

# Check for Windows-specific path handling
echo "Checking for Windows path handling..."
rg -l "(\\\\|win32|process\.platform)" --type ts

# Look for filesystem operations
echo "Checking filesystem operations..."
ast-grep --pattern 'fs.$_($$$)'

Length of output: 1666

Script:

#!/bin/bash
# Description: Inspect path construction and platform-specific logic in the codebase

# Find implementations using path.resolve, path.join, path.normalize
echo "Searching for path construction implementations..."
ast-grep --pattern 'path.$_(...)' --type ts

# Check for hard-coded path separators
echo "Checking for hard-coded path separators..."
rg -l '["\']/["\']' --type ts

# Verify conditional logic based on process.platform
echo "Searching for platform-specific conditional statements..."
rg 'if\s*\(\s*process\.platform\s*===' --type ts

Length of output: 526

Script:

#!/bin/bash
# Description: Inspect path construction and platform-specific logic in the codebase

# Find implementations using path.resolve, path.join, path.normalize
echo "Searching for path construction implementations..."
ast-grep --pattern 'path.$_(...)' -- --type ts

# Check for hard-coded path separators
echo "Checking for hard-coded path separators..."
rg -l '["/\\\']' --type ts

# Verify conditional logic based on process.platform
echo "Searching for platform-specific conditional statements..."
rg 'if\s*\(\s*process\.platform\s*===' --type ts

Length of output: 453

Script:

#!/bin/bash
# Description: Inspect path construction and platform-specific logic in the codebase

# Find implementations using path.resolve, path.join, path.normalize
echo "Searching for path construction implementations..."
ast-grep --pattern 'path.$_($_)' --lang typescript

# Check for hard-coded path separators
echo "Checking for hard-coded path separators..."
rg -l '["/\\\\\']' --type ts

# Verify conditional logic based on process.platform
echo "Searching for platform-specific conditional statements..."
rg 'if\s*\(\s*process\.platform\s*===' --type ts

Length of output: 549

Script:

#!/bin/bash
# Description: Inspect path construction and platform-specific logic in the codebase

# Find implementations using path.resolve, path.join, path.normalize
echo "Searching for path construction implementations..."
ast-grep --pattern 'path.$_($_)' --lang typescript

# Check for hard-coded path separators
echo "Checking for hard-coded path separators..."
rg -l "['\"\\\\/]" --type ts

# Verify conditional logic based on process.platform
echo "Searching for platform-specific conditional statements..."
rg 'if\s*\(\s*process\.platform\s*===' --type ts

Length of output: 1123

expect(output.html).toMatch(/\.foo/)
})
})
Copy link

@coderabbitai coderabbitai bot Nov 12, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance test coverage for filesystem path handling

The current test suite could benefit from more comprehensive coverage, particularly given the Windows path handling issues mentioned in PR #29. Consider adding these test cases:

  1. Platform-specific path testing
  2. Error handling scenarios
  3. Various path formats (relative, absolute)

Here's a suggested enhancement:

 describe('accessing file system', () => {
   it('works', async () => {
     const output = await compileToHtml('fs-access', configure, {
       path: './dist/',
       publicPath: '',
     })
     expect(output.html).toMatch(/\.foo/)
   })
+
+  it('handles Windows-style paths', async () => {
+    const output = await compileToHtml('fs-access', configure, {
+      path: '.\\dist\\',
+      publicPath: '',
+    })
+    expect(output.html).toMatch(/\.foo/)
+  })
+
+  it('handles absolute paths', async () => {
+    const output = await compileToHtml('fs-access', configure, {
+      path: path.resolve(__dirname, 'dist'),
+      publicPath: '',
+    })
+    expect(output.html).toMatch(/\.foo/)
+  })
+
+  it('handles missing stylesheet gracefully', async () => {
+    await expect(compileToHtml('fs-access', configure, {
+      path: './non-existent/',
+      publicPath: '',
+    })).rejects.toThrow()
+  })
 })

Committable suggestion skipped: line range outside the PR's diff.