feat: Public dir support #569

nathanfriemel · 2019-11-22T01:54:00Z

Took a pass at adding public folder support, looking for feedback:

Changed default config localePath to public/static/locales
Updated create config to look for combinedConfig localePath and if not found try to fall back to static/locales and print a deprecation console warning in the server config; client config attempts to remove public/ from backend paths
Updated tests

close #523

nathanfriemel · 2019-11-22T18:02:47Z

Didn't realize there was already a PR for this, feel free to close this one.

isaachinman · 2019-11-23T11:38:19Z

Hi @nathanfriemel, thanks for doing this work. I haven't heard from the author of #525 in awhile, so we can proceed with this PR if you're keen.

Can you please remove all the function return typings? I appreciate you want to fix TS warnings, but that should be handled in a separate PR as it just makes it harder to review this work. Once that's sorted I will review. Thanks!

…8next into public-dir-support

nathanfriemel · 2019-11-24T04:36:01Z

Return types are gone.

isaachinman

Overall this looks like very good work. There are some small things to change, and there are some changes in here which I'm not convinced need to be associated with this PR.

In general, I wonder if it's best to drop stuff directly in the public dir, or if we should, by convention, expect locales to be in public/static/locales. If we did that, a lot of logic could remain the same.

Let me know what you think.

isaachinman · 2019-11-25T17:17:09Z

examples/simple/package.json

-    "next-i18next": "link:../../",
-    "react": "link:../../node_modules/react",
-    "react-dom": "link:../../node_modules/react-dom"
+    "next-i18next": "link:../../"


What's going on here?

Upgrading next to version 9.0.6+ was causing the tests to throw this error. Seemed related to this bug in next, a work around was mentioned here. Wanted to test these changes against next 9.1, probably not important, could revert back to 9.0.3

Ah yeah, that's what I figured. I tried to upgrade myself, but ran into that bug.

I'm not understanding how this runs at all without react and react-dom as deps.

isaachinman · 2019-11-25T17:18:05Z

package.json

@@ -68,8 +68,8 @@
    "@types/express": "^4.16.1",
    "@types/jest": "^24.0.16",
    "@types/jest-environment-puppeteer": "^4.0.0",
-    "@types/react": "^16.8.4",
-    "@types/react-dom": "^16.8.2",
+    "@types/react": "^16.9.11",


Are these @types changes necessary? If not, let's revert that please.

isaachinman · 2019-11-25T17:18:30Z

package.json

@@ -115,6 +115,7 @@
  },
  "peerDependencies": {
    "next": ">= 7.0.0",
-    "react": ">= 16.8.0"
+    "react": ">= 16.8.0",
+    "react-dom": ">= 16.8.0"


Why was this added?

isaachinman · 2019-11-25T17:19:47Z

src/config/create-config.ts

-        throw new Error(`Default namespace not found at ${defaultNSPath}`)
+        // if defaultNS doesn't exist, try to fall back to the deprecated static folder
+        // https://github.com/isaachinman/next-i18next/issues/523
+        if (fs.existsSync(path.join(process.cwd(), `${STATIC_LOCALE_PATH}/${defaultLanguage}/${combinedConfig.defaultNS}.${localeExtension}`))) {


Can you move this logic out to a variable? Something like const staticDirExists.

isaachinman · 2019-11-25T17:20:34Z

src/config/create-config.ts

+        // if defaultNS doesn't exist, try to fall back to the deprecated static folder
+        // https://github.com/isaachinman/next-i18next/issues/523
+        if (fs.existsSync(path.join(process.cwd(), `${STATIC_LOCALE_PATH}/${defaultLanguage}/${combinedConfig.defaultNS}.${localeExtension}`))) {
+          console.warn('Deprecation Warning - falling back to /static folder, deprecated in next@9.1.*')


We've got our own logging util in src/utils/console-message.ts. Please use that where possible.

Will switch over.

isaachinman · 2019-11-25T17:21:50Z

src/config/create-config.ts

    }

  } else {

+    // remove public/ prefix from client site config
+    const clientLocalePath = localePath.replace(/^public\//, '')


I wonder how safe this is. We should probably check via something like localePath.startsWith('/public/').

Should be a no-op if the pattern doesn't match, but couldn't hurt to check startsWith first.

nathanfriemel · 2019-11-26T20:03:24Z

Changing the default to public/static/locales would probably simplify things. I'm pretty sure the Next blog suggests just moving the static folder into the public folder so that you don't have to update import paths across your app. I can make that change and probably undo a lot of the test changes in the process.

isaachinman · 2019-11-26T21:08:57Z

Sounds good. Thanks for being flexible!

…ocales, tests updated, changed next version back to 9.0.3 and undid the fixes that were needed for 9.1

isaachinman · 2019-11-28T10:56:58Z

@nathanfriemel Just let me know when this is ready for review again.

nathanfriemel · 2019-11-28T20:52:07Z

@isaachinman I think it is ready to look at again.

isaachinman

Overall looks pretty much ready to go. Can you update the README to reflect these changes?

isaachinman · 2019-11-29T12:46:07Z

__tests__/config/test-helpers.ts

@@ -15,24 +15,24 @@ const userConfig = {
  defaultNS: 'universal',
  fallbackLng: 'it',
  otherLanguages: ['fr', 'it'],
-  localePath: 'static/translations',
+  localePath: 'public/translations',


Any reason why you didn't make this public/static/translations?

Just missed it, will update it and the tests.

…ructure and updated the readme

nathanfriemel · 2019-12-02T20:34:23Z

@isaachinman README updated and updated test-helpers localePath.

isaachinman · 2019-12-03T11:26:09Z

__tests__/config/test-helpers.ts

  localeStructure: '{{ns}}/{{lng}}',
  localeSubpaths: localeSubpathVariations.FOREIGN,
 }

 const userConfigClientSide = {
  ...userConfig,
  backend: {
-    loadPath: '/static/translations/{{ns}}/{{lng}}.json',


Why was this change made?

isaachinman · 2019-12-03T11:26:35Z

__tests__/config/test-helpers.ts

  },
 }

 const userConfigServerSide = {
  ...userConfig,
  backend: {
-    loadPath: '/home/user/static/translations/{{ns}}/{{lng}}.json',
-    addPath: '/home/user/static/translations/{{ns}}/{{lng}}.missing.json',
+    loadPath: '/home/user/public/translations/{{ns}}/{{lng}}.json',


Again wondering why this isn't public/static?

It should be public/static, also the create-config.test should be changed to public/static on lines 115-116.

Looks like process.env.NODE_ENV is always 'production' in the create-config.test even with NODE_ENV=test jest so the test never runs the Validate defaultNS block and serverLocalePath is just whatever the test-helpers userConfig.localePath is set to. I'm not sure why NODE_ENV is production atm and interested to see if you see the same thing.

Yeah, it looks like it's due to this hacky setup.

We should instead capture the original value in a var, and reassign before the end of the test:

const originalNodeEnv = process.env.NODE_ENV process.env.NODE_ENV = 'production' ...perform test process.env.NODE_ENV = originalNodeEnv

Modifying globals in tests is almost always problematic!

cc @capellini

Hey @isaachinman - do you need something from me?

Just tagged you to notify you that we'll most likely need to update these tests. If @nathanfriemel is happy to do the work, I don't think we need anything!

Ok. I'm here to assist, if needed. Just let me know.

Resolved the conflicts, want me to go ahead and change static to public/static?

Went ahead and changed to public/static

src/utils/console-message.ts

yarn.lock

isaachinman · 2019-12-05T11:07:37Z

@nathanfriemel Need to resolve a couple conflicts unfortunately. But this PR is my top priority at the moment, so I won't merge anything except any potential urgent bug fixes until we get it in.

isaachinman · 2019-12-05T18:58:10Z

@nathanfriemel Let me know when this ready for re-review. Thanks!

nathanfriemel · 2019-12-05T20:35:13Z

@isaachinman Resolved conflicts, ready to look at.

isaachinman · 2019-12-11T09:22:34Z

src/config/create-config.ts

+        const staticDirExists = fs.existsSync(staticDirPath)
+        if (staticDirExists) {
+          consoleMessage('warn', 'Falling back to /static folder, deprecated in next@9.1.*', combinedConfig)
+          serverLocalePath = STATIC_LOCALE_PATH


I am just now realising that this fallback will only take place in non-production modes. Wouldn't that effectively break all currently-deployed apps where people rely on the default value of localePath being the static dir?

Yeah you're right, will look at this again in the morning.

Sorry for the break, holidays and work and what not. What was the original reason for the process.env.NODE_ENV !== 'production' here? Was it to prevent the error throw in production? I looked at the ticket in the comment and the PR for it and just want to figure out the original intent before changing anything more.

No problem. Yes that's correct - this if block is/was solely for non-production validation of the default namespace.

Moved the production check to just before the error throw, hopefully that sorts things.

isaachinman · 2019-12-24T19:22:35Z

Thank you @nathanfriemel for all your work on this. I merged the PR and cleaned up a few things, and have released under v3.0.0. There should be no breaking changes, just wanted to be on the safe side.

nathanfriemel added 3 commits November 19, 2019 13:04

upped next version to 9.1

0d32850

ts error fix

30857d5

Merge branch 'master' into public-dir-support

ce49675

nathanfriemel changed the title ~~Fixes #523, Public dir support~~ Fix: Public dir support #523 Nov 22, 2019

nathanfriemel changed the title ~~Fix: Public dir support #523~~ feature: Public dir support Nov 22, 2019

nathanfriemel changed the title ~~feature: Public dir support~~ feat: Public dir support Nov 22, 2019

nathanfriemel added 2 commits November 23, 2019 22:34

removed function return types

e15767c

Merge branch 'public-dir-support' of github.com:nathanfriemel/next-i1…

7654b43

…8next into public-dir-support

missed a couple return types

5672a4d

isaachinman suggested changes Nov 25, 2019

View reviewed changes

nathanfriemel added 4 commits November 27, 2019 13:42

using built in logging, default localePath changed to public/static/l…

3d1f4fa

…ocales, tests updated, changed next version back to 9.0.3 and undid the fixes that were needed for 9.1

Merge branch 'master' into public-dir-support

9b24341

fixing code climate errors

ce0bfd6

adding slash to header

ae482ce

isaachinman reviewed Nov 29, 2019

View reviewed changes

updated test helper localePath and related tests to include public st…

91864e8

…ructure and updated the readme

isaachinman mentioned this pull request Dec 3, 2019

feat: Add public directory support #525

Closed

isaachinman reviewed Dec 3, 2019

View reviewed changes

yarn.lock was out of sync with upstream, updating tests

38e560a

merge master

6173981

changing static to public/static

ccdc898

isaachinman suggested changes Dec 11, 2019

View reviewed changes

Merge branch 'master' into public-dir-support

9fd18f7

isaachinman mentioned this pull request Dec 22, 2019

[BUG] when you change static folder to public as recommend on nextjs last version #596

Closed

checking defaultns always and only throwing if non production

229365f

isaachinman merged commit 99f82cd into i18next:master Dec 24, 2019

feat: Public dir support #569

feat: Public dir support #569

Conversation

nathanfriemel commented Nov 22, 2019 • edited

nathanfriemel commented Nov 22, 2019

isaachinman commented Nov 23, 2019

nathanfriemel commented Nov 24, 2019

isaachinman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathanfriemel commented Nov 26, 2019

isaachinman commented Nov 26, 2019

isaachinman commented Nov 28, 2019

nathanfriemel commented Nov 28, 2019

isaachinman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathanfriemel commented Dec 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaachinman commented Dec 5, 2019

isaachinman commented Dec 5, 2019

nathanfriemel commented Dec 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaachinman commented Dec 24, 2019

nathanfriemel commented Nov 22, 2019 •

edited