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

E2E test should use the local version #2012

Merged
merged 11 commits into from
Nov 8, 2022
Merged

Conversation

belgattitude
Copy link
Contributor

@belgattitude belgattitude commented Nov 8, 2022

Before this PR our e2e tests and example install were using the last published version on npm.

This defeats the CI confidence.

As not everything is perfect, I haven't been able to simply use the npm link:../.. protocol to create a symlink to the root (where the dist folder resides). The main reason is:

Code like this:

if (!userConfig && fs.existsSync(path.resolve(DEFAULT_CONFIG_PATH))) {
userConfig = await import(path.resolve(DEFAULT_CONFIG_PATH))
}

Will be transpiled by babel (cjs) to this

 return Promise.resolve("".concat(_path["default"].resolve(DEFAULT_CONFIG_PATH))).then(function (s) {
              return _interopRequireWildcard(require(s));
            });

And it won't work in a webpack context: require(s) will fail in some situations. (I've tested a lot of options, like webpackIgnores...). Changing this is a significant work and is prone to errors (webpack have limits). Example of error:

Error occurred prerendering page "/en". Read more: https://nextjs.org/docs/messages/prerender-error
Error: Cannot find module '/home/sebastien/github/next-i18next/examples/simple/next-i18next.config.js'

See also #1917 (comment) and this https://github.com/bryantobing12/layout-reproduce-i18next/pull/1/files#diff-b09c1f3451013e7a5ceffec2636d3175563b96a911a9a91c1fe08d5110a7a1b0

My opinion (for the future):

@adrai FYI: We shouldn't play with that and always require the consuming code to provide the config (especially next 13 turbopack, etc). For example appWithTranslation and serverSideTranslations must receive the config file path. Otherwise it won't work (in monorepo situations for example). There's options to do so (like creating a new method for ssr and make the second parameter of appWithTranslation required).

In order to move forward, I'll let this for a later time.

PS: This P/R introduce more complexity for the examples. But I feel for now it's a good trade-off. I don't think people will use blindly our package.json as is (with the file:../...). The gain for us is quality.

@belgattitude
Copy link
Contributor Author

belgattitude commented Nov 8, 2022

@adrai before merging I would like your opinion on this.

TLDR: we make examples a bit dirty, we gain confidence on CI.

@@ -59,6 +59,8 @@
"example:ssg:prod": "npm run build:example:ssg && cd examples/ssg && npm run start",
"cypress": "cypress run --config-file cypress/cypress.json",
"test": "npm-run-all -s check-types clean build build:example:simple && NODE_ENV=test jest --maxWorkers=1 --silent",
"move-build-to-examples?": "Hack: see # @link https://github.com/i18next/next-i18next/pull/2012",
"move-build-to-examples": "rimraf ./examples/*/node_modules/next-i18next && cpy './package.json' './*.js' './*.ts' './dist' ./examples/simple/node_modules/next-i18next && cpy './package.json' './*.js' './*.ts' './dist' ./examples/ssg/node_modules/next-i18next",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dirty part 😮‍💨 till better options emerges.

@@ -14,7 +14,7 @@
"dependencies": {
"i18next": "^22.0.4",
"next": "^13.0.2",
"next-i18next": "^12.1.0",
"next-i18next": "file:../..",
Copy link
Contributor Author

@belgattitude belgattitude Nov 8, 2022

Choose a reason for hiding this comment

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

This makes the example less copy/pastable. But I don't think devs would use our package.json as is when looking at the example.

@adrai
Copy link
Member

adrai commented Nov 8, 2022

@adrai before merging I would like your opinion on this.

TLDR: we make examples a bit dirty, we gain confidence on CI.

I think i can live with that.

@belgattitude
Copy link
Contributor Author

Thanks I'll merge and continue a bit tomorrow. Just want to be sure the current master has no regression. I'll ping when I'm confident.

@belgattitude belgattitude merged commit cff36ee into master Nov 8, 2022
@belgattitude belgattitude deleted the e2e-use-repo-build branch November 8, 2022 23:59
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.

2 participants