sokra Github contribution chart
sokra Github Stats
sokra Most Used Languages

Activity

01 Oct 2022

Pull Request

Sokra

give output file tracing a separate spinner

Created On 01 Oct 2022 at 05:08:52
Pull Request

Sokra

EcmaScript Modules for the client build

Created On 01 Oct 2022 at 05:06:53
Pull Request

Sokra

transpile middleware utils to esm

Created On 01 Oct 2022 at 05:05:31
Merge

Sokra

Fix bundling and module resolution in the server layer

We currently resolve the react-server condition correctly inside externals for the server layer, however that will cause the resolved path to be external (as it is called "externals").

So we need a way to hook into the module resolution process to force it to use the react-server condition when it's on the server layer. The resolve option doesn't give us that ability, and the solution in this PR is to leverage normalModuleFactory's resolve hook to override the resolve options before actually resolving it. And there we can have the contextInfo.

One thing left out is bundling for the edge server, we need to add tests and sort that out carefully.

Bug

  • [ ] Related issues linked using fixes #number
  • [x] Integration tests added
  • [ ] Errors have a helpful link attached, see contributing.md

Feature

  • [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • [ ] Related issues linked using fixes #number
  • [ ] Integration tests added
  • [ ] Documentation added
  • [ ] Telemetry added. In case of a feature if it's used or not.
  • [ ] Errors have a helpful link attached, see contributing.md

Documentation / Examples

  • [ ] Make sure the linting passes by running pnpm lint
  • [ ] The "examples guidelines" are followed from our contributing doc

Forked On 23 Sep 2022 at 10:03:52

Sokra

I don't think that's the best way. Have you tried adding a `module.rules` rule which changes the resolve alias for the server layer: ``` module.rules: [ { issuerLayer: WEBPACK_LAYERS.server, resolve: { alias: { react: ..., ... } } }, ... ] ```
On 23 Sep 2022 at 10:03:52

Sokra

Fix bundling and module resolution in the server layer

We currently resolve the react-server condition correctly inside externals for the server layer, however that will cause the resolved path to be external (as it is called "externals").

So we need a way to hook into the module resolution process to force it to use the react-server condition when it's on the server layer. The resolve option doesn't give us that ability, and the solution in this PR is to leverage normalModuleFactory's resolve hook to override the resolve options before actually resolving it. And there we can have the contextInfo.

One thing left out is bundling for the edge server, we need to add tests and sort that out carefully.

Bug

  • [ ] Related issues linked using fixes #number
  • [x] Integration tests added
  • [ ] Errors have a helpful link attached, see contributing.md

Feature

  • [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • [ ] Related issues linked using fixes #number
  • [ ] Integration tests added
  • [ ] Documentation added
  • [ ] Telemetry added. In case of a feature if it's used or not.
  • [ ] Errors have a helpful link attached, see contributing.md

Documentation / Examples

  • [ ] Make sure the linting passes by running pnpm lint
  • [ ] The "examples guidelines" are followed from our contributing doc

Merged On 23 Sep 2022 at 10:04:05

Sokra

Commented On 23 Sep 2022 at 10:04:05

Sokra

Fix bundling and module resolution in the server layer

We currently resolve the react-server condition correctly inside externals for the server layer, however that will cause the resolved path to be external (as it is called "externals").

So we need a way to hook into the module resolution process to force it to use the react-server condition when it's on the server layer. The resolve option doesn't give us that ability, and the solution in this PR is to leverage normalModuleFactory's resolve hook to override the resolve options before actually resolving it. And there we can have the contextInfo.

One thing left out is bundling for the edge server, we need to add tests and sort that out carefully.

Bug

  • [ ] Related issues linked using fixes #number
  • [x] Integration tests added
  • [ ] Errors have a helpful link attached, see contributing.md

Feature

  • [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • [ ] Related issues linked using fixes #number
  • [ ] Integration tests added
  • [ ] Documentation added
  • [ ] Telemetry added. In case of a feature if it's used or not.
  • [ ] Errors have a helpful link attached, see contributing.md

Documentation / Examples

  • [ ] Make sure the linting passes by running pnpm lint
  • [ ] The "examples guidelines" are followed from our contributing doc

Merged On 23 Sep 2022 at 10:04:05

Sokra

Commented On 23 Sep 2022 at 10:04:05

Sokra

started

Started On 17 Sep 2022 at 03:38:03

Sokra

chore: Update swc

This updates SWC crates to https://github.com/swc-project/swc/commit/0c24841274fb66bac7da5c06ab98d16feadf51fa

Merged On 15 Sep 2022 at 03:09:35

Sokra

Commented On 15 Sep 2022 at 03:09:35

Sokra

chore: Update swc

This updates SWC crates to https://github.com/swc-project/swc/commit/0c24841274fb66bac7da5c06ab98d16feadf51fa

Merged On 15 Sep 2022 at 12:28:04

Sokra

Commented On 15 Sep 2022 at 12:28:04

Sokra

feat(edge): allows configuring Dynamic code execution guard

πŸ“– What's in there?

Dynamic code evaluation (eval(), new Function(), ...) is not supported on the edge runtime, hence why we fail the build when detecting such statement in the middleware or experimental-edge routes at build time.

However, there could be false positives, which static analysis and tree-shaking can not exclude:

  • qs through these dependencies (get-intrinsic: source)
  • function-bind (source)
  • has (source)

This PR leverages the existing config export to let user allow some of their files. it’s meant for allowing users to import 3rd party modules who embed dynamic code evaluation, but do not use it (because or code paths), and can't be tree-shaked.

By default, it’s keeping the existing behavior: warn in dev, fails to build. If users allow dynamic code, and that code is reached at runtime, their app stills breaks.

πŸ§ͺ How to test?

  • (existing) integration tests for disallowing dynamic code evaluation: pnpm testheadless --testPathPattern=runtime-dynamic
  • (new) integration tests for allowing dynamic code evaluation: pnpm testheadless --testPathPattern=runtime-configurable
  • (amended) production tests for validating the new configuration keys: pnpm testheadless --testPathPattern=config-validations

To try it live, you could have an application such as:

// lib/index.js
/* eslint-disable no-eval */
export function hasUnusedDynamic() {
  if ((() => false)()) {
    eval('100')
  }
}

export function hasDynamic() {
  eval('100')
}

// pages/index.jsx
export default function Page({ edgeRoute }) {
  return <p>{edgeRoute}</p>
}

export const getServerSideProps = async (req) => {
  const res = await fetch(`http://localhost:3000/api/route`)
  const data = await res.json()
  return { props: { edgeRoute: data.ok ? `Hi from the edge route` : '' } }
}

// pages/api/route.js
import { hasDynamic } from '../../lib'

export default async function handle() {
  hasDynamic()
  return Response.json({ ok: true })
}

export const config = { 
  runtime: 'experimental-edge' ,
  allowDynamic: '/lib/**'
} 

Playing with config.allowDynamic, you should be able to:

  • build the app even if it uses eval() (it will obviously fail at runtime)
  • build the app that imports but does not use eval()
  • run the app in dev, even if it uses eval() with no warning

πŸ†™ Notes to reviewers

Before adding documentation and telemetry, I'd like to collect comments on a couple of points:

  • the overall design for this feature: is a list of globs useful and easy enough?
  • should the globs be relative to the application root (current implementation) to to the edge route/middleware file?
  • (especially to @sokra) is the implementation idiomatic enough? I've leverage loaders to read the entry point configuration once, then the ModuleGraph to get it back during the parsing phase. I couldn't re-use the existing getExtractMetadata() facility since it's happening late after the parsing.
  • there's a glitch with import { ServerRuntime } from '../../types' in get-page-static-info.ts (here). I had to use next/types because it was failing during lint. Any clue why?

β˜‘οΈ Checklist

  • [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • [ ] Related issues linked using fixes #number
  • [x] Integration tests added
  • [x] Documentation added
  • [x] Telemetry added. In case of a feature if it's used or not.
  • [x] Errors have helpful link attached, see contributing.md

Merged On 12 Sep 2022 at 10:00:50

Sokra

Commented On 12 Sep 2022 at 10:00:50

Sokra

fix crash when compiler message occur

Pushed On 08 Sep 2022 at 10:37:20
Merge

Sokra

fix: support @import url in css with experimental.urlImports

fixes #37327

Just move absolute urls in @import directly into the runtime code like css-loader does.

Currently, importing css directly in js like the following will still report an error:

import 'https://cdn.jsdelivr.net/npm/bootstrap@4.4.1/dist/css/bootstrap.min.css' 

Using webpack + css-loader + style-loader directly will also have this problem.

I'm not quite sure if this usage is going to be supported, and if it is. We might need to change the stringifyRequest method in the style-loader and css-loader, so that their runtime files are resolved correctly, I tried to fix the error, but without success.

Bug

  • [x] Related issues linked using fixes #number
  • [x] Integration tests added
  • [x] Errors have helpful link attached, see contributing.md

Feature

  • [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • [ ] Related issues linked using fixes #number
  • [ ] Integration tests added
  • [ ] Documentation added
  • [ ] Telemetry added. In case of a feature if it's used or not.
  • [ ] Errors have helpful link attached, see contributing.md

Documentation / Examples

  • [] Make sure the linting passes by running pnpm lint
  • [ ] The examples guidelines are followed from our contributing doc

Forked On 08 Sep 2022 at 06:36:05

Sokra

Same. This should point to localhost.
On 08 Sep 2022 at 06:36:05
Merge

Sokra

fix: support @import url in css with experimental.urlImports

fixes #37327

Just move absolute urls in @import directly into the runtime code like css-loader does.

Currently, importing css directly in js like the following will still report an error:

import 'https://cdn.jsdelivr.net/npm/bootstrap@4.4.1/dist/css/bootstrap.min.css' 

Using webpack + css-loader + style-loader directly will also have this problem.

I'm not quite sure if this usage is going to be supported, and if it is. We might need to change the stringifyRequest method in the style-loader and css-loader, so that their runtime files are resolved correctly, I tried to fix the error, but without success.

Bug

  • [x] Related issues linked using fixes #number
  • [x] Integration tests added
  • [x] Errors have helpful link attached, see contributing.md

Feature

  • [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • [ ] Related issues linked using fixes #number
  • [ ] Integration tests added
  • [ ] Documentation added
  • [ ] Telemetry added. In case of a feature if it's used or not.
  • [ ] Errors have helpful link attached, see contributing.md

Documentation / Examples

  • [] Make sure the linting passes by running pnpm lint
  • [ ] The examples guidelines are followed from our contributing doc

Forked On 08 Sep 2022 at 06:37:40

Sokra

Returning false for a http url is incorrect when urlImports is enabled. It need to return true, since it should try to resolve the http url.
On 08 Sep 2022 at 06:37:40
Merge

Sokra

fix: support @import url in css with experimental.urlImports

fixes #37327

Just move absolute urls in @import directly into the runtime code like css-loader does.

Currently, importing css directly in js like the following will still report an error:

import 'https://cdn.jsdelivr.net/npm/bootstrap@4.4.1/dist/css/bootstrap.min.css' 

Using webpack + css-loader + style-loader directly will also have this problem.

I'm not quite sure if this usage is going to be supported, and if it is. We might need to change the stringifyRequest method in the style-loader and css-loader, so that their runtime files are resolved correctly, I tried to fix the error, but without success.

Bug

  • [x] Related issues linked using fixes #number
  • [x] Integration tests added
  • [x] Errors have helpful link attached, see contributing.md

Feature

  • [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • [ ] Related issues linked using fixes #number
  • [ ] Integration tests added
  • [ ] Documentation added
  • [ ] Telemetry added. In case of a feature if it's used or not.
  • [ ] Errors have helpful link attached, see contributing.md

Documentation / Examples

  • [] Make sure the linting passes by running pnpm lint
  • [ ] The examples guidelines are followed from our contributing doc

Forked On 08 Sep 2022 at 06:35:52

Sokra

That's not what should happen here. By using `urlImports` is should bundle the `url()` referenced resource. So it must have an url pointing to localhost. If this results in an external url the test is actually broken.
On 08 Sep 2022 at 06:35:52

Sokra

fix: support @import url in css with experimental.urlImports

fixes #37327

Just move absolute urls in @import directly into the runtime code like css-loader does.

Currently, importing css directly in js like the following will still report an error:

import 'https://cdn.jsdelivr.net/npm/bootstrap@4.4.1/dist/css/bootstrap.min.css' 

Using webpack + css-loader + style-loader directly will also have this problem.

I'm not quite sure if this usage is going to be supported, and if it is. We might need to change the stringifyRequest method in the style-loader and css-loader, so that their runtime files are resolved correctly, I tried to fix the error, but without success.

Bug

  • [x] Related issues linked using fixes #number
  • [x] Integration tests added
  • [x] Errors have helpful link attached, see contributing.md

Feature

  • [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • [ ] Related issues linked using fixes #number
  • [ ] Integration tests added
  • [ ] Documentation added
  • [ ] Telemetry added. In case of a feature if it's used or not.
  • [ ] Errors have helpful link attached, see contributing.md

Documentation / Examples

  • [] Make sure the linting passes by running pnpm lint
  • [ ] The examples guidelines are followed from our contributing doc

Merged On 08 Sep 2022 at 06:38:15

Sokra

Commented On 08 Sep 2022 at 06:38:15

Sokra

fix: support @import url in css with experimental.urlImports

fixes #37327

Just move absolute urls in @import directly into the runtime code like css-loader does.

Currently, importing css directly in js like the following will still report an error:

import 'https://cdn.jsdelivr.net/npm/bootstrap@4.4.1/dist/css/bootstrap.min.css' 

Using webpack + css-loader + style-loader directly will also have this problem.

I'm not quite sure if this usage is going to be supported, and if it is. We might need to change the stringifyRequest method in the style-loader and css-loader, so that their runtime files are resolved correctly, I tried to fix the error, but without success.

Bug

  • [x] Related issues linked using fixes #number
  • [x] Integration tests added
  • [x] Errors have helpful link attached, see contributing.md

Feature

  • [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • [ ] Related issues linked using fixes #number
  • [ ] Integration tests added
  • [ ] Documentation added
  • [ ] Telemetry added. In case of a feature if it's used or not.
  • [ ] Errors have helpful link attached, see contributing.md

Documentation / Examples

  • [] Make sure the linting passes by running pnpm lint
  • [ ] The examples guidelines are followed from our contributing doc

Merged On 08 Sep 2022 at 06:38:15

Sokra

Commented On 08 Sep 2022 at 06:38:15

Sokra

show significant percentage

Pushed On 06 Sep 2022 at 03:56:53

Sokra

more significant

Pushed On 06 Sep 2022 at 03:56:53

Sokra

run test cases interleaved to avoid per test case performance differences

Pushed On 06 Sep 2022 at 03:56:53

Sokra

move executable into temp directory

Pushed On 06 Sep 2022 at 03:56:53

Sokra

improve computation

Pushed On 06 Sep 2022 at 03:56:53

Sokra

fix crash

Pushed On 06 Sep 2022 at 03:56:53

Sokra

provide CARGO_BIN_EXE_* runtime env override to test cases

Pushed On 06 Sep 2022 at 03:56:53

Sokra

bugfixes

Pushed On 06 Sep 2022 at 03:56:53

Sokra

clear baselines before start

disable plotting

Pushed On 06 Sep 2022 at 03:56:53

Sokra

clear baselines before start

disable plotting

Pushed On 06 Sep 2022 at 03:32:26

Sokra

bugfixes

Pushed On 06 Sep 2022 at 03:17:29
Create Branch
Sokra In sokra/criterion-compare-action Create Branchsignificant2

Sokra

βš‘οΈπŸ“Š Compare the performance of Rust project branches

On 06 Sep 2022 at 02:23:48
Merge

Sokra

feat(es/minifier): Support cycles in DCE

Description:

We need graph to drop

function a() {
    b()
}
function b() {
    a()
} 

Related issue (if exists):

  • Closes https://github.com/swc-project/swc/issues/5709

Forked On 03 Sep 2022 at 09:52:38

Sokra

I feel like this renders these test cases useless and they should better run with DCE disabled for top level symbols
On 03 Sep 2022 at 09:52:38

Sokra

feat(es/minifier): Support cycles in DCE

Description:

We need graph to drop

function a() {
    b()
}
function b() {
    a()
} 

Related issue (if exists):

  • Closes https://github.com/swc-project/swc/issues/5709

Merged On 03 Sep 2022 at 09:52:39

Sokra

Commented On 03 Sep 2022 at 09:52:39
Issue Comment

Sokra

Memory leak in `vm.compileFunction` when using `importModuleDynamically`

Version

17.5.0, 16.14.0, does not happen in 14.19.0

Platform

Microsoft Windows NT 10.0.22557.0 x64

Subsystem

vm

What steps will reproduce the bug?

// test.js
const vm = require('vm')

const code = `console.log("${'hello world '.repeat(1e5)}");`

while (true) {
  for (let i = 0; i < 30; i++)
    vm.compileFunction(code, [], {
      // comment out the following line to make it no longer leaking memory
      importModuleDynamically: () => {},
    })
  if (typeof gc !== 'undefined') gc()
  console.log(
    Math.round(process.memoryUsage().heapUsed / 1024 / 10.24) / 100,
    'MiB'
  )
} 

Run this piece of code with node --expose-gc test.js.

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior?

Memory usage should stay constant.

You can also comment out the importModuleDynamically option to see the expected behavior.

Here is what I see:

4.32 MiB
4.34 MiB
4.33 MiB
4.33 MiB
4.33 MiB
4.33 MiB
4.34 MiB
4.34 MiB
4.34 MiB
4.34 MiB
4.35 MiB 

What do you see instead?

You will see memory usage increasing in an unexpected way.

Here is what I see:

5.46 MiB
5.54 MiB
5.57 MiB
5.6 MiB
5.62 MiB
5.66 MiB
5.68 MiB
5.71 MiB
5.97 MiB
5.78 MiB
5.81 MiB
5.85 MiB
5.88 MiB
5.91 MiB
5.94 MiB
5.98 MiB
6 MiB
6.03 MiB
6.06 MiB
6.09 MiB
6.12 MiB
6.15 MiB
6.19 MiB
6.22 MiB
6.25 MiB
6.28 MiB
6.31 MiB
6.34 MiB
6.37 MiB
6.4 MiB
6.43 MiB 

Additional information

There is more information in this issue: https://github.com/vercel/next.js/issues/34659#issuecomment-1047752626

I think this leak was introduced by this commit: https://github.com/nodejs/node/commit/bf2f2b74faeeb5ad7fc8e483a80bae3064e3e717#diff-c1d48dc599e8281b0be28ab449ea52917f6d4cc0578adb61ae99c631078b1a41R387

This commit could also be related: https://github.com/nodejs/node/commit/89e4b36e62978f54b2e33b4bce8197072dbe8af1

Here is the code that causes the leak in my opinion: https://github.com/nodejs/node/blob/45b5ca810a16074e639157825c1aa2e90d60e9f6/lib/vm.js#L380-L382

Here is another piece of code that could be relevant: https://github.com/nodejs/node/blob/6847fec38433a1dd16d9e3d0915e2b7fa32692c1/src/node_contextify.cc#L1197

And this piece of code: https://github.com/nodejs/node/blob/6847fec38433a1dd16d9e3d0915e2b7fa32692c1/src/node_contextify.cc#L1234-L1248

And there is this documentation about BaseObject: https://github.com/nodejs/node/tree/master/src#lifetime-management

BaseObject also defines a MakeWeak, which is not used in that case, but might be potentially relevant: https://github.com/nodejs/node/blob/470c2845cc65ac883794a1c35c962bff8d07dff2/src/base_object-inl.h#L111-L130

Forked On 23 Aug 2022 at 01:05:26

Sokra

To make any progress on this I think the issue needs to be reproducible without the --expose-gc.

It is, --expose-gc is optional for the repro. You can also run it without (which will make number more unreliable, but the leak is still there).

Commented On 23 Aug 2022 at 01:05:26
Merge

Sokra

[WIP] feat(edge): allows configuring Dynamic code execution guard

πŸ“– What's in there?

Dynamic code evaluation (eval(), new Function(), ...) is not supported on the edge runtime, hence why we fail the build when detecting such statement in the middleware or experimental-edge routes at build time.

However, there could be false positives, which static analysis and tree-shaking can not exclude:

  • qs through these dependencies (get-intrinsic: source)
  • function-bind (source)
  • has (source)

This PR leverages the existing config export to let user allow some of their files. it’s meant for allowing users to import 3rd party modules who embed dynamic code evaluation, but do not use it (because or code paths), and can't be tree-shaked.

By default, it’s keeping the existing behavior: warn in dev, fails to build. If users allow dynamic code, and that code is reached at runtime, their app stills breaks.

πŸ§ͺ How to test?

  • (existing) integration tests for disallowing dynamic code evaluation: pnpm testheadless --testPathPattern=runtime-dynamic
  • (new) integration tests for allowing dynamic code evaluation: pnpm testheadless --testPathPattern=runtime-configurable
  • (amended) production tests for validating the new configuration keys: pnpm testheadless --testPathPattern=config-validations

To try it live, you could have an application such as:

// lib/index.js
/* eslint-disable no-eval */
export function hasUnusedDynamic() {
  if ((() => false)()) {
    eval('100')
  }
}

export function hasDynamic() {
  eval('100')
}

// pages/index.jsx
export default function Page({ edgeRoute }) {
  return <p>{edgeRoute}</p>
}

export const getServerSideProps = async (req) => {
  const res = await fetch(`http://localhost:3000/api/route`)
  const data = await res.json()
  return { props: { edgeRoute: data.ok ? `Hi from the edge route` : '' } }
}

// pages/api/route.js
import { hasDynamic } from '../../lib'

export default async function handle() {
  hasDynamic()
  return Response.json({ ok: true })
}

export const config = { 
  runtime: 'experimental-edge' ,
  allowDynamic: '/lib/**'
} 

Playing with config.allowDynamic, you should be able to:

  • build the app even if it uses eval() (it will obviously fail at runtime)
  • build the app that imports but does not use eval()
  • run the app in dev, even if it uses eval() with no warning

πŸ†™ Notes to reviewers

Before adding documentation and telemetry, I'd like to collect comments on a couple of points:

  • the overall design for this feature: is a list of globs useful and easy enough?
  • should the globs be relative to the application root (current implementation) to to the edge route/middleware file?
  • (especially to @sokra) is the implementation idiomatic enough? I've leverage loaders to read the entry point configuration once, then the ModuleGraph to get it back during the parsing phase. I couldn't re-use the existing getExtractMetadata() facility since it's happening late after the parsing.
  • there's a glitch with import { ServerRuntime } from '../../types' in get-page-static-info.ts (here). I had to use next/types because it was failing during lint. Any clue why?

β˜‘οΈ Checklist

  • [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • [ ] Related issues linked using fixes #number
  • [x] Integration tests added
  • [ ] Documentation added This is needed
  • [ ] Telemetry added. In case of a feature if it's used or not. This may be needed
  • [ ] Errors have helpful link attached, see contributing.md This is needed

Forked On 22 Aug 2022 at 01:13:04

Sokra

This is only one (of potential multiple) importer path. One module might be used by multiple entrypoints with different config. You must delay error reporting until the full module graph is done.
On 22 Aug 2022 at 01:13:04
Merge

Sokra

[WIP] feat(edge): allows configuring Dynamic code execution guard

πŸ“– What's in there?

Dynamic code evaluation (eval(), new Function(), ...) is not supported on the edge runtime, hence why we fail the build when detecting such statement in the middleware or experimental-edge routes at build time.

However, there could be false positives, which static analysis and tree-shaking can not exclude:

  • qs through these dependencies (get-intrinsic: source)
  • function-bind (source)
  • has (source)

This PR leverages the existing config export to let user allow some of their files. it’s meant for allowing users to import 3rd party modules who embed dynamic code evaluation, but do not use it (because or code paths), and can't be tree-shaked.

By default, it’s keeping the existing behavior: warn in dev, fails to build. If users allow dynamic code, and that code is reached at runtime, their app stills breaks.

πŸ§ͺ How to test?

  • (existing) integration tests for disallowing dynamic code evaluation: pnpm testheadless --testPathPattern=runtime-dynamic
  • (new) integration tests for allowing dynamic code evaluation: pnpm testheadless --testPathPattern=runtime-configurable
  • (amended) production tests for validating the new configuration keys: pnpm testheadless --testPathPattern=config-validations

To try it live, you could have an application such as:

// lib/index.js
/* eslint-disable no-eval */
export function hasUnusedDynamic() {
  if ((() => false)()) {
    eval('100')
  }
}

export function hasDynamic() {
  eval('100')
}

// pages/index.jsx
export default function Page({ edgeRoute }) {
  return <p>{edgeRoute}</p>
}

export const getServerSideProps = async (req) => {
  const res = await fetch(`http://localhost:3000/api/route`)
  const data = await res.json()
  return { props: { edgeRoute: data.ok ? `Hi from the edge route` : '' } }
}

// pages/api/route.js
import { hasDynamic } from '../../lib'

export default async function handle() {
  hasDynamic()
  return Response.json({ ok: true })
}

export const config = { 
  runtime: 'experimental-edge' ,
  allowDynamic: '/lib/**'
} 

Playing with config.allowDynamic, you should be able to:

  • build the app even if it uses eval() (it will obviously fail at runtime)
  • build the app that imports but does not use eval()
  • run the app in dev, even if it uses eval() with no warning

πŸ†™ Notes to reviewers

Before adding documentation and telemetry, I'd like to collect comments on a couple of points:

  • the overall design for this feature: is a list of globs useful and easy enough?
  • should the globs be relative to the application root (current implementation) to to the edge route/middleware file?
  • (especially to @sokra) is the implementation idiomatic enough? I've leverage loaders to read the entry point configuration once, then the ModuleGraph to get it back during the parsing phase. I couldn't re-use the existing getExtractMetadata() facility since it's happening late after the parsing.
  • there's a glitch with import { ServerRuntime } from '../../types' in get-page-static-info.ts (here). I had to use next/types because it was failing during lint. Any clue why?

β˜‘οΈ Checklist

  • [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • [ ] Related issues linked using fixes #number
  • [x] Integration tests added
  • [ ] Documentation added This is needed
  • [ ] Telemetry added. In case of a feature if it's used or not. This may be needed
  • [ ] Errors have helpful link attached, see contributing.md This is needed

Forked On 22 Aug 2022 at 01:14:26

Sokra

There is no (valid) way to get the entrypoint config during parsing. This check must be delayed until the full graph is available.
On 22 Aug 2022 at 01:14:26

Sokra

[WIP] feat(edge): allows configuring Dynamic code execution guard

πŸ“– What's in there?

Dynamic code evaluation (eval(), new Function(), ...) is not supported on the edge runtime, hence why we fail the build when detecting such statement in the middleware or experimental-edge routes at build time.

However, there could be false positives, which static analysis and tree-shaking can not exclude:

  • qs through these dependencies (get-intrinsic: source)
  • function-bind (source)
  • has (source)

This PR leverages the existing config export to let user allow some of their files. it’s meant for allowing users to import 3rd party modules who embed dynamic code evaluation, but do not use it (because or code paths), and can't be tree-shaked.

By default, it’s keeping the existing behavior: warn in dev, fails to build. If users allow dynamic code, and that code is reached at runtime, their app stills breaks.

πŸ§ͺ How to test?

  • (existing) integration tests for disallowing dynamic code evaluation: pnpm testheadless --testPathPattern=runtime-dynamic
  • (new) integration tests for allowing dynamic code evaluation: pnpm testheadless --testPathPattern=runtime-configurable
  • (amended) production tests for validating the new configuration keys: pnpm testheadless --testPathPattern=config-validations

To try it live, you could have an application such as:

// lib/index.js
/* eslint-disable no-eval */
export function hasUnusedDynamic() {
  if ((() => false)()) {
    eval('100')
  }
}

export function hasDynamic() {
  eval('100')
}

// pages/index.jsx
export default function Page({ edgeRoute }) {
  return <p>{edgeRoute}</p>
}

export const getServerSideProps = async (req) => {
  const res = await fetch(`http://localhost:3000/api/route`)
  const data = await res.json()
  return { props: { edgeRoute: data.ok ? `Hi from the edge route` : '' } }
}

// pages/api/route.js
import { hasDynamic } from '../../lib'

export default async function handle() {
  hasDynamic()
  return Response.json({ ok: true })
}

export const config = { 
  runtime: 'experimental-edge' ,
  allowDynamic: '/lib/**'
} 

Playing with config.allowDynamic, you should be able to:

  • build the app even if it uses eval() (it will obviously fail at runtime)
  • build the app that imports but does not use eval()
  • run the app in dev, even if it uses eval() with no warning

πŸ†™ Notes to reviewers

Before adding documentation and telemetry, I'd like to collect comments on a couple of points:

  • the overall design for this feature: is a list of globs useful and easy enough?
  • should the globs be relative to the application root (current implementation) to to the edge route/middleware file?
  • (especially to @sokra) is the implementation idiomatic enough? I've leverage loaders to read the entry point configuration once, then the ModuleGraph to get it back during the parsing phase. I couldn't re-use the existing getExtractMetadata() facility since it's happening late after the parsing.
  • there's a glitch with import { ServerRuntime } from '../../types' in get-page-static-info.ts (here). I had to use next/types because it was failing during lint. Any clue why?

β˜‘οΈ Checklist

  • [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • [ ] Related issues linked using fixes #number
  • [x] Integration tests added
  • [ ] Documentation added This is needed
  • [ ] Telemetry added. In case of a feature if it's used or not. This may be needed
  • [ ] Errors have helpful link attached, see contributing.md This is needed

Merged On 22 Aug 2022 at 01:14:30

Sokra

Commented On 22 Aug 2022 at 01:14:30

Sokra

[WIP] feat(edge): allows configuring Dynamic code execution guard

πŸ“– What's in there?

Dynamic code evaluation (eval(), new Function(), ...) is not supported on the edge runtime, hence why we fail the build when detecting such statement in the middleware or experimental-edge routes at build time.

However, there could be false positives, which static analysis and tree-shaking can not exclude:

  • qs through these dependencies (get-intrinsic: source)
  • function-bind (source)
  • has (source)

This PR leverages the existing config export to let user allow some of their files. it’s meant for allowing users to import 3rd party modules who embed dynamic code evaluation, but do not use it (because or code paths), and can't be tree-shaked.

By default, it’s keeping the existing behavior: warn in dev, fails to build. If users allow dynamic code, and that code is reached at runtime, their app stills breaks.

πŸ§ͺ How to test?

  • (existing) integration tests for disallowing dynamic code evaluation: pnpm testheadless --testPathPattern=runtime-dynamic
  • (new) integration tests for allowing dynamic code evaluation: pnpm testheadless --testPathPattern=runtime-configurable
  • (amended) production tests for validating the new configuration keys: pnpm testheadless --testPathPattern=config-validations

To try it live, you could have an application such as:

// lib/index.js
/* eslint-disable no-eval */
export function hasUnusedDynamic() {
  if ((() => false)()) {
    eval('100')
  }
}

export function hasDynamic() {
  eval('100')
}

// pages/index.jsx
export default function Page({ edgeRoute }) {
  return <p>{edgeRoute}</p>
}

export const getServerSideProps = async (req) => {
  const res = await fetch(`http://localhost:3000/api/route`)
  const data = await res.json()
  return { props: { edgeRoute: data.ok ? `Hi from the edge route` : '' } }
}

// pages/api/route.js
import { hasDynamic } from '../../lib'

export default async function handle() {
  hasDynamic()
  return Response.json({ ok: true })
}

export const config = { 
  runtime: 'experimental-edge' ,
  allowDynamic: '/lib/**'
} 

Playing with config.allowDynamic, you should be able to:

  • build the app even if it uses eval() (it will obviously fail at runtime)
  • build the app that imports but does not use eval()
  • run the app in dev, even if it uses eval() with no warning

πŸ†™ Notes to reviewers

Before adding documentation and telemetry, I'd like to collect comments on a couple of points:

  • the overall design for this feature: is a list of globs useful and easy enough?
  • should the globs be relative to the application root (current implementation) to to the edge route/middleware file?
  • (especially to @sokra) is the implementation idiomatic enough? I've leverage loaders to read the entry point configuration once, then the ModuleGraph to get it back during the parsing phase. I couldn't re-use the existing getExtractMetadata() facility since it's happening late after the parsing.
  • there's a glitch with import { ServerRuntime } from '../../types' in get-page-static-info.ts (here). I had to use next/types because it was failing during lint. Any clue why?

β˜‘οΈ Checklist

  • [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • [ ] Related issues linked using fixes #number
  • [x] Integration tests added
  • [ ] Documentation added This is needed
  • [ ] Telemetry added. In case of a feature if it's used or not. This may be needed
  • [ ] Errors have helpful link attached, see contributing.md This is needed

Merged On 22 Aug 2022 at 01:14:30

Sokra

Commented On 22 Aug 2022 at 01:14:30
Issue Comment

Sokra

fetch response during server rendering

Bug

  • [ ] Related issues linked using fixes #number
  • [ ] Integration tests added
  • [ ] Errors have helpful link attached, see contributing.md

Feature

  • [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • [ ] Related issues linked using fixes #number
  • [ ] Integration tests added
  • [ ] Documentation added
  • [ ] Telemetry added. In case of a feature if it's used or not.
  • [ ] Errors have helpful link attached, see contributing.md

Documentation / Examples

  • [ ] Make sure the linting passes by running pnpm lint
  • [ ] The examples guidelines are followed from our contributing doc

Forked On 19 Aug 2022 at 11:07:30

Sokra

We should also check the env var for only doing that at build time, so it doesn't affect getServerSideProps

Commented On 19 Aug 2022 at 11:07:30
Issue Comment

Sokra

fetch response during server rendering

Bug

  • [ ] Related issues linked using fixes #number
  • [ ] Integration tests added
  • [ ] Errors have helpful link attached, see contributing.md

Feature

  • [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
  • [ ] Related issues linked using fixes #number
  • [ ] Integration tests added
  • [ ] Documentation added
  • [ ] Telemetry added. In case of a feature if it's used or not.
  • [ ] Errors have helpful link attached, see contributing.md

Documentation / Examples

  • [ ] Make sure the linting passes by running pnpm lint
  • [ ] The examples guidelines are followed from our contributing doc

Forked On 19 Aug 2022 at 10:40:38

Sokra

@SukkaW good points, do you want to continue this PR? This was a random idea @ijjk and I had at 3am at the offsite meeting.

It could potentially improve the performance of quite a few users. Seems like it's quite common that getStaticProps for page instances shares some fetches. So this potentially improves build time quite a lot for these cases.

Commented On 19 Aug 2022 at 10:40:38
Pull Request

Sokra

fetch response during server rendering

Created On 19 Aug 2022 at 01:21:23