Skip to content
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.

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

Fix checking /_app #43

Merged
merged 1 commit into from
Jul 25, 2023
Merged

Fix checking /_app #43

merged 1 commit into from
Jul 25, 2023

Conversation

TheMatrixan
Copy link
Contributor

@TheMatrixan TheMatrixan commented Jun 21, 2023

Fixes: #35 aralroca/next-translate#1116

[ ] Documentation update
[+] Bug fix
[ ] New feature
[ ] Other, please explain:

What changes did you make? (Give an overview)

Which issue (if any) does this pull request address?

Is there anything you'd like reviewers to focus on?

src/loader.ts Outdated
@@ -116,14 +116,14 @@ export default function loader(
//
// This way, the only modified file has to be the _app.js.
if (hasGetInitialPropsOnAppJs) {
return pageNoExt === '/_app'
return pageNoExt.startsWith('/_app')
Copy link
Owner

Choose a reason for hiding this comment

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

the problem is that this is not going to work in monorepos...

Copy link
Owner

@aralroca aralroca Jun 21, 2023

Choose a reason for hiding this comment

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

well, maybe yes is fine. If before was just === and was working... pageNoExt is only the page and not all the path... So then yes. This is to fix these cases /_app.page.ts, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is a fix for _app.page.tsx - custom pageExtensions.

Copy link
Owner

@aralroca aralroca left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! LGTM. The only thing... can you add a test similar than this?

it('should return templateWithHoc', () => {
mockHasStaticName.mockReturnValueOnce(true)
const code = `
import withSomeHoc from 'some-hoc'
export default function Page() {
return <h1>Page</h1>
}
Page.getInitialProps = () => ({ props: {} })
`
loader.call({ ...config, resourcePath: 'pages/some-page.ts' }, code)
expect(mockTemplateWithHoc).toBeCalled()
})

Thanks!!

@TheMatrixan
Copy link
Contributor Author

Okay, done, @aralroca

@aralroca aralroca merged commit 0af914f into aralroca:main Jul 25, 2023
6 checks passed
@aralroca
Copy link
Owner

aralroca commented Jul 25, 2023

@TheMatrixan thanks for your contribution. Your code is already available in 2.5.3-canary.2 prerelase

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix checking /_app in the plugin
2 participants