Skip to content

Commit

Permalink
fix(babel): Improved message for error relating to multiple files end…
Browse files Browse the repository at this point in the history
…ing in `Page.{js,jsx,ts,tsx}` in page directories (#9329)

**Problem**
The route auto import babel plugin currently generates duplicate import
statements if you have multiple files which end in
`Page.{js,jsx,ts,tsx}` within the same page directory. This causes an
error later in the build/dev process. This currently imposes the
restriction that only one file end in `Page.{js,jsx,ts,tsx}` inside the
individual page directories.

The error message which you get when this issue occurs is rather obscure
and does not easily point users to the source of the problem. This was
highlighted in #9165.

The error produced from `yarn rw dev` without this change:
<img width="931" alt="image"
src="https://github.com/redwoodjs/redwood/assets/56300765/9e7abda6-d42e-477c-8506-b9caea2f1fc8">

In comparison to after this change:
<img width="931" alt="image"
src="https://github.com/redwoodjs/redwood/assets/56300765/964ca266-338d-4fbb-af0f-b75c8f6647ac">

**Changes**
1. Checks for duplicate pages as returned from the `processPagesDir`
function and produces a more specific error message in the case where
duplicates exist.
2. Adds a test which covers the case of duplicate files ending in
`Page.{js,jsx,ts,tsx}` within a page directory.

**Fixes**
* fixes #9165 

**Notes**
1. The `processPagesDir` function is marked as deprecated and is
awaiting a new implementation. I did not think doing that now for this
issue was wise. I also did not want to alter the logic of that function
here as that could introduce side effects of its own.
2. Happy to change the error message to be even clearer.
3. We could attempt to trim out the duplicate entries directly where I
perform this new check - I'm not against this. I didn't do this
immediately because it felt like I would be doing additional work which
the `processPagesDir` function should be doing internally.
  • Loading branch information
Josh-Walker-GM authored Oct 23, 2023
1 parent 433d8ef commit 9d4a1e7
Show file tree
Hide file tree
Showing 6 changed files with 83 additions and 5 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
[web]
port = 8910
apiProxyPath = "/api/functions"

[api]
port = 8911
[api.paths]
functions = './api/src/functions'
graphql = './api/src/graphql'
generated = './api/generated'
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { Router, Route } from '@redwoodjs/router'

const Routes = () => {
return (
<Router>
<Route path="/" page={HomePage} name="home"/>
</Router>
)
}

export default Routes
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
const HomePage = () => {
return (
<div>
<h1>HomePage</h1>
</div>
)
}

export default HomePage
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
const useHomePage = () => {
return 'useHomePage'
}

export default useHomePage
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,6 @@ import { getPaths } from '@redwoodjs/project-config'

import babelRoutesAutoLoader from '../babel-plugin-redwood-routes-auto-loader'

const FIXTURE_PATH = path.resolve(
__dirname,
'../../../../../__fixtures__/example-todo-main/'
)

const transform = (filename: string) => {
const code = fs.readFileSync(filename, 'utf-8')
return babel.transform(code, {
Expand All @@ -21,7 +16,35 @@ const transform = (filename: string) => {
})
}

describe('mulitiple files ending in Page.{js,jsx,ts,tsx}', () => {
const FAILURE_FIXTURE_PATH = path.resolve(
__dirname,
'./__fixtures__/route-auto-loader/failure'
)

beforeAll(() => {
process.env.RWJS_CWD = FAILURE_FIXTURE_PATH
})

afterAll(() => {
delete process.env.RWJS_CWD
})

test('Fails with appropriate message', () => {
expect(() => {
transform(getPaths().web.routes)
}).toThrowError(
"Unable to find only a single file ending in 'Page.{js,jsx,ts,tsx}' in the follow page directories: 'HomePage"
)
})
})

describe('page auto loader correctly imports pages', () => {
const FIXTURE_PATH = path.resolve(
__dirname,
'../../../../../__fixtures__/example-todo-main/'
)

let result: babel.BabelFileResult | null

beforeAll(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,26 @@ export default function (
// @NOTE: This var gets mutated inside the visitors
let pages = processPagesDir().map(withRelativeImports)

// Currently processPagesDir() can return duplicate entries when there are multiple files
// ending in Page in the individual page directories. This will cause an error upstream.
// Here we check for duplicates and throw a more helpful error message.
const duplicatePageImportNames = new Set<string>()
const sortedPageImportNames = pages.map((page) => page.importName).sort()
for (let i = 0; i < sortedPageImportNames.length - 1; i++) {
if (sortedPageImportNames[i + 1] === sortedPageImportNames[i]) {
duplicatePageImportNames.add(sortedPageImportNames[i])
}
}
if (duplicatePageImportNames.size > 0) {
throw new Error(
`Unable to find only a single file ending in 'Page.{js,jsx,ts,tsx}' in the follow page directories: ${Array.from(
duplicatePageImportNames
)
.map((name) => `'${name}'`)
.join(', ')}`
)
}

return {
name: 'babel-plugin-redwood-routes-auto-loader',
visitor: {
Expand Down

0 comments on commit 9d4a1e7

Please sign in to comment.