fix(babel): Improved message for error relating to multiple files ending in Page.{js,jsx,ts,tsx} in page directories
#9329
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jtoar
approved these changes
Oct 23, 2023
| // 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 duplicatePageImports = new Set<string>() | ||
| const sortedPageImports = pages.map((page) => page.importName).sort() |
There was a problem hiding this comment.
Minor suggesiton to improve the naming of this variable here cause it's not import statements its just an array of strings like ['AboutPage', 'AboutPage', 'FatalErrorPage']
Suggested change
| const sortedPageImports = pages.map((page) => page.importName).sort() | |
| const sortedPageImportNames = pages.map((page) => page.importName).sort() |
There was a problem hiding this comment.
Agreed. Updating in ce11673
jtoar
pushed a commit
that referenced
this pull request
Nov 2, 2023
…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.
jtoar
pushed a commit
that referenced
this pull request
Nov 3, 2023
…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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 inPage.{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 devwithout this change:In comparison to after this change:
Changes
processPagesDirfunction and produces a more specific error message in the case where duplicates exist.Page.{js,jsx,ts,tsx}within a page directory.Fixes
Notes
processPagesDirfunction 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.processPagesDirfunction should be doing internally.