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

Update initialize_bouncer_middleware.stub #13

Closed
wants to merge 1 commit into from

Conversation

brunolipe-a
Copy link
Contributor

πŸ”— Linked issue

❓ Type of change

  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

There is a typescript build problem in this code. I don't know if this is the best way to do it, but it is resolves the problem.

image

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@Julien-R44
Copy link
Member

Hey, thanks for the PR, but I think you are just missing this provider in your adonisrc.ts file :

  providers: [
     // ... others providers
    () => import('@adonisjs/core/providers/edge_provider'),
  ],

@Julien-R44 Julien-R44 closed this Jan 25, 2024
@brunolipe-a brunolipe-a deleted the patch-2 branch January 25, 2024 22:15
@brunolipe-a
Copy link
Contributor Author

So we should add that to configure command?

@Julien-R44
Copy link
Member

Oh wait sorry. Indeed a fix is needed, because maybe you are not using Edge. And so, ctx.view type will be unknown. But using a as any will not be the best way. We gonna think about it

@thetutlage
Copy link
Member

If you are not using edge You just delete that line from the created middleware.

There is no need to modify the stub.

@brunolipe-a
Copy link
Contributor Author

So this should be explicit in the docs, right? So that we don't have this same issue in the future.

@thetutlage
Copy link
Member

Yeah, we can update the docs to mention to remove this line if you are not using Edge.

@Julien-R44
Copy link
Member

Julien-R44 commented Jan 26, 2024

What about conditionally add this line in the middleware only if edge.js is installed on the user's application? I mean, in configure.ts we could have a check like that
It should be easy to do

@RomainLanz
Copy link
Member

What about conditionally add this line in the middleware only if edge.js is installed on the user's application? I mean, in configure.ts we could have a check like that It should be easy to do

If we do that, people will install Edge.js afterward will have an issue. I believe we are better off documenting it and leave it to the user.

@Julien-R44
Copy link
Member

i was thinking about that : if Edge is not installed, we add the middleware with commented code. that way, if people need to install edge afterward, they can simply uncomment the code

    /**
     * Uncomment if you need Edge helpers
     */
   // if ('view' in ctx) {
   //  ctx.view.share(ctx.bouncer.edgeHelpers)
   // }

Having to manually update the bouncer middleware seems a bit boring. But it's not the end of the world either

@thetutlage
Copy link
Member

I think, we are trying to serve individuals who do not want to understand the surrounding ecosystem of the framework. They will always struggle to use the framework.

  • You have a piece of code which gives a compiler error
  • Then within the docs, the same piece of code is documented with the reasoning "Why it exists" and "When to remove it".

But you ignore the docs, do not understand the error in the first place and start with an issue, then its a problem with you and not with anything else.

Also, I do not want to make these configure commands complicated like in v5 and then create a mess for ourselves in order to maintain them.

@thetutlage
Copy link
Member

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.

4 participants