Commit to AsyncLocalStorage #216
Conversation
🦋 Changeset detectedLatest commit: ae14c00 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
🧪 A prerelease is available for testing 🧪 You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/next-on-pages/runs/4926135844/npm-package-next-on-pages-216Or you can immediately run this with npx https://prerelease-registry.devprod.cloudflare.dev/next-on-pages/runs/4926135844/npm-package-next-on-pages-216 |
dario-piotrowicz
force-pushed
the
commit-to-als
branch
2 times, most recently
from
eb5eb6c
to
085969b
Compare
dario-piotrowicz
force-pushed
the
commit-to-als
branch
from
085969b
to
cbc3395
Compare
There was a problem hiding this comment.
It would be cool to have a test which actually evaluates the ALS store. That might be outwith the capabilities of the current test setup, but if you can think of a way to actually run the code, that might be worthwhile.
There was a problem hiding this comment.
yeah you're totally right but I think this will get more than covered by our e2es
let me have a play with this anyways 🤔 (maybe I can just call the function and use eval in my test? 🤷)
There was a problem hiding this comment.
sorry I can't test it with eval because if fails because of the ALS import:
In a followup PR I want to change the import to be dynamic anyways (like so: 42c6b79) so I can try to unit test this then?
(if all fails we will still have the e2e? 😅)
| globalThis.AsyncLocalStorage = AsyncLocalStorage; | ||
|
|
||
| const __ENV_ALS__ = new AsyncLocalStorage(); | ||
| __ENV_ALS__['NODE_ENV'] = '${nodeEnv}'; |
There was a problem hiding this comment.
Is that definitely correct? I'm still getting familiar with ALS APIs, but it seems a bit weird to be setting a value directly on the ALS instance here, rather than on the store.
Does __ENV_ALS__.getStore() return something that references back to props assigned on the __ENV_ALS__ instance somehow?
Double-check that process.env.NODE_ENV and process.env.SOME_OTHER_SET_ENV_VAR are set properly in an example project.
There was a problem hiding this comment.
nono, to be totally clear, __ENV_ALS__['NODE_ENV'] is a hack, I just wanted to have the NODE_ENV available and didn't want to create a new variable for it, so I am just piggybacking it on top of the ASL (as a plain object)
(
you can see here:
)
Alternatively I can have a __NODE_ENV__ variable instead (I don't think I can use it in the ASL initialization)
There was a problem hiding this comment.
PS: I did check that I get all the process.env variables in an example project 😅 (specifically here: https://github.com/dario-piotrowicz/next-apps-for-testing/blob/802e9478a3b322f9b2850008aec9e4abd7bc8215/apps/simple-app-dir-13.3.1/app/api/hello/route.js)
There was a problem hiding this comment.
ohhh... yeah, I am not a fan of that, I'm afraid. Why not just do __ENV_ALS__.run({ ...env, NODE_ENV: ${nodeEnv} })?
There was a problem hiding this comment.
Ah, because it's in a separate unprocessed file. I'd maybe use esbuild's define feature then.
__ENV_ALS__.run({ ...env, NODE_ENV: __NODE_ENV__ })
and define({ __NODE_ENV__: "production" }) in the esbuild command.
There was a problem hiding this comment.
Ah, in fact you've changed it even more. I'm just now seeing getNodeEnv() too.
Okay, whatever. Last suggestion would be to put it in the proxy maybe?
get (_, property) => {
if (property === 'NODE_ENV') {
return ${nodeEnv}
} else {
return Reflect.get(__ENV_ALS__.getStore(), property)
}
}
But not the end of the world if you ship what you have right now.
There was a problem hiding this comment.
Okay, whatever. Last suggestion would be to put it in the proxy maybe?
I totally thought of this, and I don't really like the idea because it prevents the user code to update the value, it is quite unlikely (maybe they can do it for testing and whatnot?) but I wouldn't really want to change how things work and potentially introduce quirky behaviors
But I do think I can just esbuild’s define (sorry I didn’t think of that 😓)
There was a problem hiding this comment.
done! (97b1afb)
Thank you so very much for the suggestion! 🙏 (and sorry for the silly mistake 😓)
| @@ -24,7 +24,7 @@ export function constructBuildOutputRecord(item: BuildOutputItem) { | |||
| }` | |||
| : `{ | |||
| type: ${JSON.stringify(item.type)}, | |||
| entrypoint: AsyncLocalStoragePromise.then(() => import('${item.entrypoint}')) | |||
| entrypoint: import('${item.entrypoint}') | |||
There was a problem hiding this comment.
here I'm keeping the dynamic import as that is relevant for the lazy loading
| globalThis.AsyncLocalStorage = AsyncLocalStorage; | ||
|
|
||
| const __ENV_ALS__ = new AsyncLocalStorage(); | ||
| __ENV_ALS__['NODE_ENV'] = '${nodeEnv}'; |
There was a problem hiding this comment.
I wasn't sure what to do with the NODE_ENV so I am just piggybacking it to the ASL object, alternatively I could make the get below return the nodeEnv value on NODE_ENV but that feels hackier (since for example would break things if someone tried updating that, as I guess could happen with testing frameworks?)
There was a problem hiding this comment.
yeah you're totally right but I think this will get more than covered by our e2es
let me have a play with this anyways 🤔 (maybe I can just call the function and use eval in my test? 🤷)
| globalThis.AsyncLocalStorage = AsyncLocalStorage; | ||
|
|
||
| const __ENV_ALS__ = new AsyncLocalStorage(); | ||
| __ENV_ALS__['NODE_ENV'] = '${nodeEnv}'; |
There was a problem hiding this comment.
nono, to be totally clear, __ENV_ALS__['NODE_ENV'] is a hack, I just wanted to have the NODE_ENV available and didn't want to create a new variable for it, so I am just piggybacking it on top of the ASL (as a plain object)
(
you can see here:
)
Alternatively I can have a __NODE_ENV__ variable instead (I don't think I can use it in the ASL initialization)
| globalThis.AsyncLocalStorage = AsyncLocalStorage; | ||
|
|
||
| const __ENV_ALS__ = new AsyncLocalStorage(); | ||
| __ENV_ALS__['NODE_ENV'] = '${nodeEnv}'; |
There was a problem hiding this comment.
PS: I did check that I get all the process.env variables in an example project 😅 (specifically here: https://github.com/dario-piotrowicz/next-apps-for-testing/blob/802e9478a3b322f9b2850008aec9e4abd7bc8215/apps/simple-app-dir-13.3.1/app/api/hello/route.js)
resolves #121