feat(jsconfig-paths-plugin): Resolve paths synchronously #29467

jantimon · 2021-09-28T15:42:54Z

Feature

Allows linaria to statically extract css-in-js without running into Cannot 'resolveSync' because the fileSystem is not sync

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
fixes change jsconfig-paths-plugin to resolve paths synchronously #29410
fixes change jsconfig-paths-plugin to resolve paths synchronously #29421
~~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

Btw triedPaths is unused.. exactly like before.
Should we remove it?

fixes vercel#29410 fixes vercel#29421

jantimon · 2021-10-08T09:55:41Z

@sokra can you please take a look?

sokra

Only reviewed on mobile, so there might be more...

packages/next/build/webpack/plugins/jsconfig-paths-plugin.ts

ijjk · 2021-10-10T10:09:21Z

Failing test suites

Commit: 39997ed

test/integration/jsconfig-paths/test/index.test.js

TypeScript Features > default behavior > should alias components
TypeScript Features > default behavior > should resolve the first item in the array first
TypeScript Features > default behavior > should resolve the second item as fallback
TypeScript Features > default behavior > should resolve a single matching alias
TypeScript Features > default behavior > should resolve a wildcard alias
TypeScript Features > default behavior > should have correct module not found error

Expand output

● TypeScript Features › default behavior › should alias components

thrown: "Exceeded timeout of 90000 ms for a hook.
Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

  25 |     let output = ''
  26 |
> 27 |     beforeAll(async () => {
     |     ^
  28 |       appPort = await findPort()
  29 |       app = await launchApp(appDir, appPort, {
  30 |         onStderr(msg) {

  at integration/jsconfig-paths/test/index.test.js:27:5
  at integration/jsconfig-paths/test/index.test.js:24:3
  at Object.<anonymous> (integration/jsconfig-paths/test/index.test.js:23:1)

● TypeScript Features › default behavior › should resolve the first item in the array first

thrown: "Exceeded timeout of 90000 ms for a hook.
Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

  25 |     let output = ''
  26 |
> 27 |     beforeAll(async () => {
     |     ^
  28 |       appPort = await findPort()
  29 |       app = await launchApp(appDir, appPort, {
  30 |         onStderr(msg) {

  at integration/jsconfig-paths/test/index.test.js:27:5
  at integration/jsconfig-paths/test/index.test.js:24:3
  at Object.<anonymous> (integration/jsconfig-paths/test/index.test.js:23:1)

● TypeScript Features › default behavior › should resolve the second item as fallback

thrown: "Exceeded timeout of 90000 ms for a hook.
Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

  25 |     let output = ''
  26 |
> 27 |     beforeAll(async () => {
     |     ^
  28 |       appPort = await findPort()
  29 |       app = await launchApp(appDir, appPort, {
  30 |         onStderr(msg) {

  at integration/jsconfig-paths/test/index.test.js:27:5
  at integration/jsconfig-paths/test/index.test.js:24:3
  at Object.<anonymous> (integration/jsconfig-paths/test/index.test.js:23:1)

● TypeScript Features › default behavior › should resolve a single matching alias

thrown: "Exceeded timeout of 90000 ms for a hook.
Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

  25 |     let output = ''
  26 |
> 27 |     beforeAll(async () => {
     |     ^
  28 |       appPort = await findPort()
  29 |       app = await launchApp(appDir, appPort, {
  30 |         onStderr(msg) {

  at integration/jsconfig-paths/test/index.test.js:27:5
  at integration/jsconfig-paths/test/index.test.js:24:3
  at Object.<anonymous> (integration/jsconfig-paths/test/index.test.js:23:1)

● TypeScript Features › default behavior › should resolve a wildcard alias

thrown: "Exceeded timeout of 90000 ms for a hook.
Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

  25 |     let output = ''
  26 |
> 27 |     beforeAll(async () => {
     |     ^
  28 |       appPort = await findPort()
  29 |       app = await launchApp(appDir, appPort, {
  30 |         onStderr(msg) {

  at integration/jsconfig-paths/test/index.test.js:27:5
  at integration/jsconfig-paths/test/index.test.js:24:3
  at Object.<anonymous> (integration/jsconfig-paths/test/index.test.js:23:1)

● TypeScript Features › default behavior › should have correct module not found error

thrown: "Exceeded timeout of 90000 ms for a hook.
Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

  25 |     let output = ''
  26 |
> 27 |     beforeAll(async () => {
     |     ^
  28 |       appPort = await findPort()
  29 |       app = await launchApp(appDir, appPort, {
  30 |         onStderr(msg) {

  at integration/jsconfig-paths/test/index.test.js:27:5
  at integration/jsconfig-paths/test/index.test.js:24:3
  at Object.<anonymous> (integration/jsconfig-paths/test/index.test.js:23:1)

● Test suite failed to run

TypeError: Cannot read property 'pid' of undefined

  329 | export async function killApp(instance) {
  330 |   await new Promise((resolve, reject) => {
> 331 |     treeKill(instance.pid, (err) => {
      |                       ^
  332 |       if (err) {
  333 |         if (
  334 |           process.platform === 'win32' &&

  at lib/next-test-utils.js:331:23
  at Object.killApp (lib/next-test-utils.js:330:9)
  at integration/jsconfig-paths/test/index.test.js:12:25

test/integration/gssp-ssr-change-reloading/test/index.test.js

GS(S)P Server-Side Change Reloading > should update page when getStaticProps is changed only
GS(S)P Server-Side Change Reloading > should show indicator when re-fetching data
GS(S)P Server-Side Change Reloading > should update page when getStaticPaths is changed only
GS(S)P Server-Side Change Reloading > should not reload page when client-side is changed too GSSP
GS(S)P Server-Side Change Reloading > should update page when getServerSideProps is changed only

Expand output

● GS(S)P Server-Side Change Reloading › should update page when getStaticProps is changed only

TIMED OUT: 2

1

  437 |
  438 |   if (hardError) {
> 439 |     throw new Error('TIMED OUT: ' + regex + '\n\n' + content)
      |           ^
  440 |   }
  441 |   return false
  442 | }

  at Object.check (lib/next-test-utils.js:439:11)
  at Object.<anonymous> (integration/gssp-ssr-change-reloading/test/index.test.js:78:5)

● GS(S)P Server-Side Change Reloading › should show indicator when re-fetching data

expect(received).toBe(expected) // Object.is equality

Expected: 1
Received: 2

  93 |     expect(await browser.eval(() => window.beforeChange)).toBe('hi')
  94 |     expect(await browser.eval(() => window.showedBuilder)).toBe(true)
> 95 |     page.restore()
     |                 ^
  96 |
  97 |     await check(
  98 |       async () =>

  at Object.<anonymous> (integration/gssp-ssr-change-reloading/test/index.test.js:95:17)

● GS(S)P Server-Side Change Reloading › should update page when getStaticPaths is changed only

expect(received).toBe(expected) // Object.is equality

Expected: 1
Received: 2

  87 |
  88 |     await check(
> 89 |       async () =>
     |                  ^
  90 |         JSON.parse(await browser.elementByCss('#props').text()).count + '',
  91 |       '2'
  92 |     )

  at Object.<anonymous> (integration/gssp-ssr-change-reloading/test/index.test.js:89:29)

● GS(S)P Server-Side Change Reloading › should not reload page when client-side is changed too GSSP

Failed to replace content.

Pattern: change me

Content: export default function Gssp(props) {
  return (
    <>
      <p id="change">changed</p>
      <p id="props">{JSON.stringify(props)}</p>
    </>
  )
}

export const getServerSideProps = ({ params }) => {
  const count = 2

  return {
    props: {
      count,
      params,
      random: Math.random(),
    },
  }
}

  467 |     } else if (typeof pattern === 'string') {
  468 |       if (!currentContent.includes(pattern)) {
> 469 |         throw new Error(
      |               ^
  470 |           `Failed to replace content.\n\nPattern: ${pattern}\n\nContent: ${currentContent}`
  471 |         )
  472 |       }

  at File.replace (lib/next-test-utils.js:469:15)
  at Object.<anonymous> (integration/gssp-ssr-change-reloading/test/index.test.js:163:63)

● GS(S)P Server-Side Change Reloading › should update page when getServerSideProps is changed only

expect(received).toBe(expected) // Object.is equality

Expected: 1
Received: 2

  185 |       async () =>
  186 |         JSON.parse(await browser.elementByCss('#props').text()).count + '',
> 187 |       '1'
      |          ^
  188 |     )
  189 |   })
  190 | })

  at Object.<anonymous> (integration/gssp-ssr-change-reloading/test/index.test.js:187:10)

test/integration/typescript-paths/test/index.test.js

TypeScript Features > default behavior > should alias components
TypeScript Features > default behavior > should resolve the first item in the array first
TypeScript Features > default behavior > should resolve the second item in as a fallback
TypeScript Features > default behavior > should resolve a single matching alias
TypeScript Features > default behavior > should not resolve to .d.ts files

Expand output

● TypeScript Features › default behavior › should alias components

thrown: "Exceeded timeout of 90000 ms for a test.
Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

  22 |     afterAll(() => killApp(app))
  23 |
> 24 |     it('should alias components', async () => {
     |     ^
  25 |       const $ = await get$('/basic-alias')
  26 |       expect($('body').text()).toMatch(/World/)
  27 |     })

  at integration/typescript-paths/test/index.test.js:24:5
  at integration/typescript-paths/test/index.test.js:17:3
  at Object.<anonymous> (integration/typescript-paths/test/index.test.js:16:1)

● TypeScript Features › default behavior › should resolve the first item in the array first

thrown: "Exceeded timeout of 90000 ms for a test.
Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

  27 |     })
  28 |
> 29 |     it('should resolve the first item in the array first', async () => {
     |     ^
  30 |       const $ = await get$('/resolve-order')
  31 |       expect($('body').text()).toMatch(/Hello from a/)
  32 |     })

  at integration/typescript-paths/test/index.test.js:29:5
  at integration/typescript-paths/test/index.test.js:17:3
  at Object.<anonymous> (integration/typescript-paths/test/index.test.js:16:1)

● TypeScript Features › default behavior › should resolve the second item in as a fallback

thrown: "Exceeded timeout of 90000 ms for a test.
Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

  32 |     })
  33 |
> 34 |     it('should resolve the second item in as a fallback', async () => {
     |     ^
  35 |       const $ = await get$('/resolve-fallback')
  36 |       expect($('body').text()).toMatch(/Hello from only b/)
  37 |     })

  at integration/typescript-paths/test/index.test.js:34:5
  at integration/typescript-paths/test/index.test.js:17:3
  at Object.<anonymous> (integration/typescript-paths/test/index.test.js:16:1)

● TypeScript Features › default behavior › should resolve a single matching alias

thrown: "Exceeded timeout of 90000 ms for a test.
Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

  37 |     })
  38 |
> 39 |     it('should resolve a single matching alias', async () => {
     |     ^
  40 |       const $ = await get$('/single-alias')
  41 |       expect($('body').text()).toMatch(/Hello/)
  42 |     })

  at integration/typescript-paths/test/index.test.js:39:5
  at integration/typescript-paths/test/index.test.js:17:3
  at Object.<anonymous> (integration/typescript-paths/test/index.test.js:16:1)

● TypeScript Features › default behavior › should not resolve to .d.ts files

thrown: "Exceeded timeout of 90000 ms for a test.
Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

  42 |     })
  43 |
> 44 |     it('should not resolve to .d.ts files', async () => {
     |     ^
  45 |       const $ = await get$('/alias-to-d-ts')
  46 |       expect($('body').text()).toMatch(/Not aliased to d\.ts file/)
  47 |     })

  at integration/typescript-paths/test/index.test.js:44:5
  at integration/typescript-paths/test/index.test.js:17:3
  at Object.<anonymous> (integration/typescript-paths/test/index.test.js:16:1)

jantimon · 2021-10-11T07:17:42Z

hey @sokra
thanks for reviewing the code and finding the bug which sneaked in..

could you please review once again - now that the pipeline is green?

ijjk · 2021-10-11T19:34:28Z

Stats from current PR

Default Build (Decrease detected ✓)

General Overall increase ⚠️

	vercel/next.js canary	jantimon/next.js fix-linaria-resolving	Change
buildDuration	15.6s	16.2s	⚠️ +616ms
buildDurationCached	3.5s	3.6s	⚠️ +51ms
nodeModulesSize	195 MB	195 MB	⚠️ +1.14 kB

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	jantimon/next.js fix-linaria-resolving	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.656	3.651	-0.01
/ avg req/sec	683.87	684.72	+0.85
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.81	1.85	⚠️ +0.04
/error-in-render avg req/sec	1381.25	1351.19	⚠️ -30.06

Client Bundles (main, webpack, commons)

	vercel/next.js canary	jantimon/next.js fix-linaria-resolving	Change
779.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.2 kB	42.2 kB	✓
main-HASH.js gzip	27 kB	27 kB	✓
webpack-HASH.js gzip	1.45 kB	1.45 kB	✓
Overall change	70.9 kB	70.9 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	jantimon/next.js fix-linaria-resolving	Change
polyfills-a4..dd70.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	jantimon/next.js fix-linaria-resolving	Change
_app-HASH.js gzip	977 B	977 B	✓
_error-HASH.js gzip	194 B	194 B	✓
amp-HASH.js gzip	311 B	311 B	✓
css-HASH.js gzip	328 B	328 B	✓
dynamic-HASH.js gzip	2.67 kB	2.67 kB	✓
head-HASH.js gzip	351 B	351 B	✓
hooks-HASH.js gzip	918 B	918 B	✓
image-HASH.js gzip	4.12 kB	4.12 kB	✓
index-HASH.js gzip	260 B	260 B	✓
link-HASH.js gzip	1.66 kB	1.66 kB	✓
routerDirect..HASH.js gzip	320 B	320 B	✓
script-HASH.js gzip	386 B	386 B	✓
withRouter-HASH.js gzip	319 B	319 B	✓
bb14e60e810b..30f.css gzip	125 B	125 B	✓
Overall change	12.9 kB	12.9 kB	✓

Client Build Manifests

	vercel/next.js canary	jantimon/next.js fix-linaria-resolving	Change
_buildManifest.js gzip	494 B	494 B	✓
Overall change	494 B	494 B	✓

Rendered Page Sizes

	vercel/next.js canary	jantimon/next.js fix-linaria-resolving	Change
index.html gzip	539 B	539 B	✓
link.html gzip	551 B	551 B	✓
withRouter.html gzip	532 B	532 B	✓
Overall change	1.62 kB	1.62 kB	✓

Default Build with SWC (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	jantimon/next.js fix-linaria-resolving	Change
buildDuration	8.2s	7.4s	-766ms
buildDurationCached	3.5s	3.8s	⚠️ +346ms
nodeModulesSize	195 MB	195 MB	⚠️ +1.14 kB

Page Load Tests Overall increase ✓

	vercel/next.js canary	jantimon/next.js fix-linaria-resolving	Change
/ failed reqs	0	0	✓
/ total time (seconds)	3.536	3.552	⚠️ +0.02
/ avg req/sec	707.1	703.74	⚠️ -3.36
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.872	1.823	-0.05
/error-in-render avg req/sec	1335.19	1371.37	+36.18

Client Bundles (main, webpack, commons)

	vercel/next.js canary	jantimon/next.js fix-linaria-resolving	Change
675-HASH.js gzip	13.8 kB	13.8 kB	✓
770.HASH.js gzip	178 B	178 B	✓
framework-HASH.js gzip	50.7 kB	50.7 kB	✓
main-HASH.js gzip	35 kB	35 kB	✓
webpack-HASH.js gzip	1.64 kB	1.64 kB	✓
Overall change	101 kB	101 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	jantimon/next.js fix-linaria-resolving	Change
polyfills-a4..dd70.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	jantimon/next.js fix-linaria-resolving	Change
_app-HASH.js gzip	1.33 kB	1.33 kB	✓
_error-HASH.js gzip	180 B	180 B	✓
amp-HASH.js gzip	315 B	315 B	✓
css-HASH.js gzip	331 B	331 B	✓
dynamic-HASH.js gzip	2.79 kB	2.79 kB	✓
head-HASH.js gzip	355 B	355 B	✓
hooks-HASH.js gzip	637 B	637 B	✓
image-HASH.js gzip	555 B	555 B	✓
index-HASH.js gzip	261 B	261 B	✓
link-HASH.js gzip	2.22 kB	2.22 kB	✓
routerDirect..HASH.js gzip	326 B	326 B	✓
script-HASH.js gzip	393 B	393 B	✓
withRouter-HASH.js gzip	322 B	322 B	✓
bb14e60e810b..30f.css gzip	125 B	125 B	✓
Overall change	10.1 kB	10.1 kB	✓

Client Build Manifests

	vercel/next.js canary	jantimon/next.js fix-linaria-resolving	Change
_buildManifest.js gzip	511 B	511 B	✓
Overall change	511 B	511 B	✓

Rendered Page Sizes

	vercel/next.js canary	jantimon/next.js fix-linaria-resolving	Change
index.html gzip	537 B	537 B	✓
link.html gzip	551 B	551 B	✓
withRouter.html gzip	532 B	532 B	✓
Overall change	1.62 kB	1.62 kB	✓

Commit: ae3641e

sokra · 2021-10-13T07:11:38Z

packages/next/build/webpack/plugins/jsconfig-paths-plugin.ts

-                  resolve([resolverErr, resolverResult])
+                (resolverErr: any, resolverResult: any) => {
+                  if (resolverErr || resolverResult === undefined) {
+                    triedPaths.push(candidate)


triedPaths doesn't seem to be used at all...

sokra · 2021-10-13T10:17:56Z

Thanks

timneutkens · 2021-10-13T12:00:02Z

This PR is being published in https://github.com/vercel/next.js/releases/tag/v11.1.3-canary.66

jantimon · 2021-10-14T13:50:15Z

Cool thanks! :)

feat(jsconfig-paths-plugin): Resolve paths synchronously

d8506c2

fixes vercel#29410 fixes vercel#29421

jantimon requested review from huozhi, ijjk, padmaia, shuding, styfle and timneutkens as code owners September 28, 2021 15:42

ijjk added the type: next label Sep 28, 2021

jantimon added 5 commits September 28, 2021 17:43

Merge branch 'canary' into fix-linaria-resolving

cf595a9

copy forEachBail

2fff99a

Merge branch 'canary' into fix-linaria-resolving

084f794

Merge branch 'canary' into fix-linaria-resolving

a29067f

Merge branch 'canary' into fix-linaria-resolving

39997ed

sokra requested changes Oct 10, 2021

View reviewed changes

packages/next/build/webpack/plugins/jsconfig-paths-plugin.ts Outdated Show resolved Hide resolved

packages/next/build/webpack/plugins/jsconfig-paths-plugin.ts Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

jantimon added 2 commits October 10, 2021 18:34

retry next path candidates

c1dcef6

Merge branch 'canary' into fix-linaria-resolving

83674aa

This comment has been minimized.

Sign in to view

Merge branch 'canary' into fix-linaria-resolving

464651b

jantimon requested a review from sokra October 11, 2021 07:53

Merge branch 'canary' into fix-linaria-resolving

ae3641e

sokra approved these changes Oct 13, 2021

View reviewed changes

sokra merged commit 31a701b into vercel:canary Oct 13, 2021

ijjk added the hacktoberfest-accepted label Oct 14, 2021

vercel locked as resolved and limited conversation to collaborators Jan 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(jsconfig-paths-plugin): Resolve paths synchronously #29467

feat(jsconfig-paths-plugin): Resolve paths synchronously #29467

jantimon commented Sep 28, 2021

jantimon commented Oct 8, 2021

sokra left a comment

This comment has been minimized.

ijjk commented Oct 10, 2021 •

edited

Loading

This comment has been minimized.

jantimon commented Oct 11, 2021

ijjk commented Oct 11, 2021

sokra Oct 13, 2021

sokra commented Oct 13, 2021

timneutkens commented Oct 13, 2021

jantimon commented Oct 14, 2021

feat(jsconfig-paths-plugin): Resolve paths synchronously #29467

feat(jsconfig-paths-plugin): Resolve paths synchronously #29467

Conversation

jantimon commented Sep 28, 2021

Feature

Documentation / Examples

jantimon commented Oct 8, 2021

sokra left a comment

Choose a reason for hiding this comment

This comment has been minimized.

ijjk commented Oct 10, 2021 • edited Loading

Failing test suites

This comment has been minimized.

jantimon commented Oct 11, 2021

ijjk commented Oct 11, 2021

Stats from current PR

sokra Oct 13, 2021

Choose a reason for hiding this comment

sokra commented Oct 13, 2021

timneutkens commented Oct 13, 2021

jantimon commented Oct 14, 2021

ijjk commented Oct 10, 2021 •

edited

Loading