Add named exports for createInstance

[perrin4869]

Updating i18next-fetch-backend to be an esm-first module, I found that I could not do import { createInstance } from "i18next" like I expected.
This PR adds support for this.

[jamuhl]

Don't find the issue but we had issues with mixed export in the past.

[jamuhl]

for reference https://github.com/i18next/i18next/blob/master/CHANGELOG.md#1700

[coveralls]

Coverage increased (+0.07%) to 90.871% when pulling 40d814b on perrin4869-patch-1 into 9ad73d5 on master.

[perrin4869]

Oh I see! Yeah that would not have worked then.
I tried a different approach here, it adds named exports only on the esm module and marks it as type: "module" so that it can be loaded natively by node.js.

[jamuhl]

@adrai what is your opinion? I think either we keep the thing as is - or bring back all named exports (personally I think named exports are overused on things that are objects and not pure functional)

[adrai]

Personally, I would not mix default exports and named exports...
The only thing we could do is better deconstruction support... like spiked here: https://github.com/adrai/i18next-next/blob/main/src/i18next.js#L624

This way you could do something like this:

import i18next from 'i18next';
const { createInstance } = i18next;

[perrin4869]

createInstance btw is just a pure function, it even has a rule to disable eslint above it.

  /* eslint class-methods-use-this: 0 */
  createInstance(options = {}, callback) {
    return new I18n(options, callback);
  }

I still think this one can be safely added as a named export...

[jamuhl]

with pure function I mean it is just a function (not part of a class)

[perrin4869]

Is there a reason createInstance is attached to a class's prototype? Is there a need to call it from a class instance?

[jamuhl]

instance.createInstance is a pattern used since long before deconstruction was a thing

[jamuhl]

Not even sure why this is such a big deal...most projects never will need createInstance anyway...there is nothing you gain by the named import

[perrin4869]

You're absolutely right, it's not a big deal. It's just a preference man. You gain slightly nicer ergonomics.
I'm doing routine maintenance of some of my modules, and when updating i18next-fetch-backend I ran into this.
https://github.com/dotcore64/i18next-fetch-backend/blob/c30759a7703d63e365b7e4e9500e832724cc5ad6/test/index.js#L11
It just looks off, and it's something that I could quickly fix.
I also confirmed that node16 can load this module as native ESM with no troubles.

I think that if anything, the pattern would be to make createInstance a static method... I think that @babel/preset-env supports this out of the box now...
Feel free to close this if it's a no-go!

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[perrin4869]

Solved the merge conflicts and updated to reflect #1682. I actually agree with @jamuhl and don't particularly like #1682, originally all I wanted was to be able to import { createInstance } from 'i18next' XD
but since that one was merged, I went ahead and made the ESM module behave like the cjs. I also moved the binding outside of the constructor, so that only the members of the exported i18next instance are bound... having all functions bound to a member is just not intuitive...

[adrai]

outside of the constructor

This will probably not automatically work for cloneInstance and createInstance now: https://github.com/i18next/i18next/blob/master/src/i18next.js#L569

[perrin4869]

Hm... I think there would be no problem calling those 2...
Nevertheless, it doesn't make much sense to bind cloneInstance to begin with - it's right there in the name :P
And createInstance i changed into a static method and it is bound separately.

[adrai]

I mean the bindMemberFunctions call is probably also needed to be called here: https://github.com/i18next/i18next/pull/1681/files#diff-6859f2a371e0f563ec9c27c5cf64cb82f8d91750467509caaa8f6f0e6d06dec5R565

[perrin4869]

Ah, i see what you mean! I did that on purpose, i think binding on the default export instance is enough...
Do you think it's better to bind all instances?
I don't like the idea cause it could lead to unexpected or unintuitive behavior...

[adrai]

Ah, i see what you mean! I did that on purpose, i think binding on the default export instance is enough...
Do you think it's better to bind all instances?
I don't like the idea cause it could lead to unexpected or unintuitive behavior...

what do you think @joshkel @dinosaurjoe @nielsboecker @minodisk @stephent?

[adrai]

lgtm now...
just waiting for @joshkel's feedback for the bind question: #1681 (comment)

[joshkel]

For my particular use case, I believe that binding on the default instance would be enough.

I would lean toward binding all instances; that feels more consistent and removes any possibility of error. But I'll defer to whatever others decide. Thank you.

[adrai]

I would lean toward binding all instances; that feels more consistent and removes any possibility of error. But I'll defer to whatever others decide. Thank you.

So let's do it for all instance creations ok @perrin4869 ?

[perrin4869]

Done!
I think that some people might not expect the instance functions to be bound, in general people don't bind their instances... don't know of other libs that do this... but let's go with this!

[adrai]

thank you for everything you've done @perrin4869 v21.5.0 has just been released

[stryaponoff]

I've got the warning after upgrading to v21.5.0 with Yarn 3.1.0:

warn Package i18next has been ignored because it contains invalid configuration. Reason: Package subpath './package.json' is not defined by "exports"

But it seems to work anyway without any problems.

[adrai]

I've got the warning after upgrading to v21.5.0 with Yarn 3.1.0:

warn Package i18next has been ignored because it contains invalid configuration. Reason: Package subpath './package.json' is not defined by "exports"

But it seems to work anyway without any problems.

@stryaponoff it should be better with v21.5.1