fix(esbuild): remove all node: imports from glob script to keep support for Jest v26
#5784
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
christian-bromann
changed the title
node: imports from glob script to keep support for Jest v26
PR built and packed!Download the tarball here: https://github.com/ionic-team/stencil/actions/runs/9176899440/artifacts/1523378555 If your browser saves files to |
|
| Path | Error Count |
|---|---|
| src/dev-server/index.ts | 37 |
| src/dev-server/server-process.ts | 32 |
| src/compiler/prerender/prerender-main.ts | 22 |
| src/runtime/client-hydrate.ts | 20 |
| src/runtime/vdom/vdom-render.ts | 20 |
| src/screenshot/connector-base.ts | 19 |
| src/testing/puppeteer/puppeteer-element.ts | 19 |
| src/dev-server/request-handler.ts | 15 |
| src/compiler/prerender/prerender-optimize.ts | 14 |
| src/compiler/sys/stencil-sys.ts | 14 |
| src/runtime/connected-callback.ts | 14 |
| src/sys/node/node-sys.ts | 14 |
| src/compiler/prerender/prerender-queue.ts | 13 |
| src/compiler/sys/in-memory-fs.ts | 13 |
| src/runtime/set-value.ts | 13 |
| src/compiler/output-targets/output-www.ts | 12 |
| src/compiler/transformers/test/parse-vdom.spec.ts | 12 |
| src/compiler/transformers/transform-utils.ts | 12 |
| src/mock-doc/test/attribute.spec.ts | 12 |
| src/compiler/build/compiler-ctx.ts | 11 |
Our most common errors
| Typescript Error Code | Count |
|---|---|
| TS2322 | 358 |
| TS2345 | 344 |
| TS18048 | 201 |
| TS18047 | 77 |
| TS2722 | 37 |
| TS2532 | 24 |
| TS2531 | 19 |
| TS2454 | 14 |
| TS2790 | 11 |
| TS2352 | 9 |
| TS2769 | 8 |
| TS2416 | 7 |
| TS2538 | 4 |
| TS2493 | 3 |
| TS18046 | 2 |
| TS2684 | 1 |
| TS2430 | 1 |
Unused exports report
There are 15 unused exports on this PR. That's the same number of errors on main, so at least we're not creating new ones!
Unused exports
| File | Line | Identifier |
|---|---|---|
| src/runtime/bootstrap-lazy.ts | 21 | setNonce |
| src/screenshot/screenshot-fs.ts | 18 | readScreenshotData |
| src/testing/testing-utils.ts | 198 | withSilentWarn |
| src/utils/index.ts | 145 | CUSTOM |
| src/utils/index.ts | 245 | NODE_TYPES |
| src/utils/index.ts | 269 | normalize |
| src/utils/index.ts | 7 | escapeRegExpSpecialCharacters |
| src/compiler/app-core/app-data.ts | 25 | BUILD |
| src/compiler/app-core/app-data.ts | 116 | Env |
| src/compiler/app-core/app-data.ts | 118 | NAMESPACE |
| src/compiler/fs-watch/fs-watch-rebuild.ts | 123 | updateCacheFromRebuild |
| src/compiler/types/validate-primary-package-output-target.ts | 61 | satisfies |
| src/compiler/types/validate-primary-package-output-target.ts | 61 | Record |
| src/testing/puppeteer/puppeteer-declarations.ts | 485 | WaitForEventOptions |
| src/compiler/sys/fetch/write-fetch-success.ts | 7 | writeFetchSuccessSync |
Comment on lines
+104
to
+106
| const globOutputPath = join(opts.output.sysNodeDir, 'glob.js'); | ||
| const glob = fs.readFileSync(globOutputPath, 'utf8'); | ||
| fs.writeFileSync(globOutputPath, glob.replace(/require\("node:/g, 'require("')); |
There was a problem hiding this comment.
Is it better to just revert glob and block that upgrade until we drop Jest 26 support (in v5)?
There was a problem hiding this comment.
Unfortunately that wouldn't work as glob itself is not the culprit here, it's glob dependencies, e.g. path-scurry, that bring in these node imports. If we would use pnpm we could use the overwrite or in Yarn we could use resolutions but there is nothing in NPM to do this. Hence tweaking the build artifact seemed like the easiest approach.
alicewriteswrongs
approved these changes
May 22, 2024
tanner-reits
approved these changes
May 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is the current behavior?
With Stencil v4.18.1 Stencil broke support for Jest v26 due to the fact that transient dependencies for
globinsys/node/glob.jsstarted to use imports withnode:prefixes.GitHub Issue Number: #5766
What is the new behavior?
I enhanced the sys node build script to remove these prefixes. This won't have any impact on the script and allows us to keep support for Jest v26.
Documentation
Does this introduce a breaking change?
Testing
pnpm installand verify that test pass withnpm testno such file or directory, open 'node:path'@stencil/core@4.18.2-dev.1716246842.b6713e1(dev build of this branch) and rerun tests, verify that tests passOther information
I was under the assumption that we are testing running tests in Jest v26, I will take a look if we could have caught this before release.