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

Add new helpers sub-module with getRequestExecutionContext #450

Closed
wants to merge 1 commit into from

Conversation

Karibash
Copy link
Contributor

@Karibash Karibash commented Sep 7, 2023

Add a helper function where ExecutionContext can be retrieved.
If you don't like this API, it's not a problem to close it.

We are currently using it in our project, and it's working well.
https://github.com/looks-to-me/looks-to-me/blob/3b5393ecc95e6e1af3e8bbd949ba5a246a23ec47/app/src/app/(main)/images/posts/%5Bid%5D/route.ts#L60

This implementation is based on the following pull request for reference.

@changeset-bot
Copy link

changeset-bot bot commented Sep 7, 2023

🦋 Changeset detected

Latest commit: 488ab56

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@cloudflare/next-on-pages Minor
eslint-plugin-next-on-pages Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Karibash Karibash changed the title Add new helpers sum-module with getRequestExecutionContext Add new helpers sub-module with getRequestExecutionContext Sep 7, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2023

🧪 Prereleases are available for testing 🧪

@cloudflare/next-on-pages

You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/next-on-pages/runs/6497892763/npm-package-next-on-pages-450

@cloudflare/eslint-plugin-next-on-pages

You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/next-on-pages/runs/6497892763/npm-package-eslint-plugin-next-on-pages-450

@dario-piotrowicz
Copy link
Member

Thanks a lot for the PR @Karibash 🙂

Sorry I haven't updated you in #233, anyways as I mentioned there your solution seems valid but I think we might go a different direction, maybe your solution could be a temporary one if nothing else but it depends on how things proceed with the other alternatives (if we can soon go with the team's preferred alternative we might just go straight to that one)

On our side there aren't many updates (I've also been OOO for a couple of weeks) I'll need to chase the team up and try to get to the bottom of things, sorry for this taking a long time 😓

@dihmeetree
Copy link

Any updates on this? :)

@dario-piotrowicz
Copy link
Member

Hi @dihmeetree, sorry no, not yet, I've still waiting to see what happens with cloudflare/workerd#1213

Since, if that runtime API were to be released then any changes in next-on-pages would be unnecessary.

(But the PR has been stale for a while and I am not sure if we are going to land it 😓)

Let's wait a bit longer and see what happens.

Also the changes in this PR do not give access to the request object which is also something that users might need access to, so I am not too sure about it 🤔

I'll try to speed things up with the team and see if the runtime API is going to be added or not, if it's not let's discuss further here and adjust the API accordingly 🙂 (I also need to check with @GregBrimble as I think he's also interested in this and I think he's considering a potential generic/non next-on-pages spoecific approach).

(PS: so sorry this it taken so long 😓)

@dario-piotrowicz
Copy link
Member

Hi @Karibash, I wanted to let you know, eventually (finally! 😅) we've decided to implement a getRequestContext utility (#659) that exposes ctx as well as env and cf.

The utility basically works the exact same way yours does and is implemented in a similar way.

Sorry for going ahead and opening a new PR for it, I hope you don't mind too much 🙇

Please have a look to see if like it 🙂


PS: since we've got #659 I'm closing this PR, I hope that's fine, if not please feel free to reopen it and discuss

@Karibash Karibash deleted the feature/execution-context branch February 7, 2024 10:04
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.

3 participants