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

localeNegotiator with custom context #2

Merged
merged 1 commit into from
Mar 2, 2022
Merged

localeNegotiator with custom context #2

merged 1 commit into from
Mar 2, 2022

Conversation

EdJoPaTo
Copy link
Member

Using the localeNegotiator doesnt work with a custom context currently.
This is needed to access things like ctx.session (like the examples in the README are suggesting).

There does not seem to be a code formatting specified, it differs inside the files and no npm script for it. Maybe use deno fmt in the future?

I test changes with npm pack and install the created package in another project. PrepublishOnly does not work for npm pack. prepack does also work for publishing a package.

@slavafomin
Copy link
Member

Hello!

Thank you for your contribution.

Using the localeNegotiator doesnt work with a custom context currently.

I don't believe this is the case, actually. If you would type the argument of the localeNegotiator function manually, it will work correctly. I have this code in my project, which works:

bot.use(useFluent({
    fluent,
    localeNegotiator: async (context: CustomContext) => {
      const user = await context.getUser();
      return (
        user.locale ||
        context.from.language_code ||
        defaultLocale
      );
    }
  }));

However, having the ability to specify ContextType to the useFluent() function is convenient, so thanks.

There does not seem to be a code formatting specified, it differs inside the files and no npm script for it. Maybe use deno fmt in the future?

I'm planning to add ESLint in the future with customized set of rules. I don't really like the opinionated linting tools.

I test changes with npm pack and install the created package in another project. PrepublishOnly does not work for npm pack. prepack does also work for publishing a package.

I wasn't aware of such workflow. It's a good fix. I think I will switch from using prepublishOnly to prepack in all my projects now. Thanks.

By the way, I would suggest you to use npm link, it is quite convenient workflow, when you are working on small projects and libraries. The code changes will get reflected instantly, you don't need to run npm pack all the time.

@EdJoPaTo
Copy link
Member Author

Using the localeNegotiator doesnt work with a custom context currently.

I don't believe this is the case, actually. If you would type the argument of the localeNegotiator function manually, it will work correctly. I have this code in my project, which works:

This does not work for me:

source/bot/index.ts:51:2 - error TS2322: Type '(ctx: MyContext) => string' is not assignable to type 'LocaleNegotiator<Context>'.
  Types of parameters 'ctx' and 'context' are incompatible.
    Type 'Context' is not assignable to type 'MyContext'.
      Property 'session' is missing in type 'Context' but required in type 'SessionFlavor<Session>'.

51  localeNegotiator: (ctx: MyContext) => ctx.session.__language_code ?? ctx.from?.language_code ?? 'en',
    ~~~~~~~~~~~~~~~~

  node_modules/grammy/out/convenience/session.d.ts:29:9
    29     get session(): S;
               ~~~~~~~
    'session' is declared here.
  node_modules/@moebius/grammy-fluent/dist/types/middleware.d.ts:7:5
    7     localeNegotiator?: LocaleNegotiator;
          ~~~~~~~~~~~~~~~~
    The expected type comes from property 'localeNegotiator' which is declared here on type 'GrammyFluentOptions'


Found 1 error.

I test changes with npm pack and install the created package in another project. PrepublishOnly does not work for npm pack. prepack does also work for publishing a package.

I wasn't aware of such workflow. It's a good fix. I think I will switch from using prepublishOnly to prepack in all my projects now. Thanks.

By the way, I would suggest you to use npm link, it is quite convenient workflow, when you are working on small projects and libraries. The code changes will get reflected instantly, you don't need to run npm pack all the time.

npm link has corner cases where the code is breaking due to being installed this way. Telegraf for example could not be tested via npm link in all cases. npm pack basically allows for testing with the exact conditions as installed via npmjs.com. Also npm link links it into the system which requires the superuser or permissions to the given direction which is something I avoid. Everything should be able to run as user without more permissions than necessary.

@slavafomin
Copy link
Member

slavafomin commented Jan 25, 2022

Yep, it's controlled by the strictFunctionTypes TSC flag. You can test it here:
https://www.typescriptlang.org/play?strictFunctionTypes=false#code/JYOwLgpgTgZghgYwgAgMIHtwQB5mQbwChkTkYp0BbALgIF8AaQuwwsATwAcUAZdBOABsIASQAmyALzIAzmCigA5gG5WHbsj4DhAOQiL0YYHDDooAHmJpMkXABUuKHJBBiZ1rLikfbYVgD5vAAoEG2daDE8wB24ASilAoK0hUQkAH2QABQpKYBkIHmAAawhzZOFxf1jVQlBIWEQUAHEoOEpKdgAxQQBXCHAAeU4jTHciEkF+FL0DIxMzAH5acogZw2NTKFUWQhgekAQRkGQe-O6+8CCrdGHgUdoWto7z-rAho5lWeKIdgHpfqSSIHAkGgsHgySsOrQeBIZAAWXYkV8yGc-TcPmcBCsJAAFhBBJNaHIFCAVMxWKcIC9LuNSJNtKt9Ot5lBaCEwrhaIjkc54pJAnTSKQoBAwD0oMcAOT9KWqYWMZjVQhAA

In any case, good call! This will make library code more type safe and easier to use.


Well, I'm using nvm so there is no problems with superuser permissions, because Node is installed completely in userland.

The only problem I know with npm link and tsc is that some node_modules that are used both in library and in the host project could be resolved incorrectly, but it's quite easy to fix with tsc paths option.

@slavafomin
Copy link
Member

By the way I will merge this PQ once we finish migrating this repository. I hope later today.

@EdJoPaTo
Copy link
Member Author

I didnt notice the new version as the name changed (@grammyjs/fluent). But even with the new version this problem still persists and this PR is still open. Am I missing something? Was it just forgotten?

@slavafomin slavafomin merged commit ee59dcc into grammyjs:main Mar 2, 2022
@slavafomin
Copy link
Member

slavafomin commented Mar 2, 2022

@EdJoPaTo thanks again for your contribution and sorry that it took so long to merge :)

I've published the updated version.

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.

None yet

2 participants