-
Notifications
You must be signed in to change notification settings - Fork 794
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
feat(compiler): deprecate customResolveOptions config option #5269
feat(compiler): deprecate customResolveOptions config option #5269
Conversation
|
Path | Error Count |
---|---|
src/dev-server/index.ts | 37 |
src/mock-doc/serialize-node.ts | 36 |
src/dev-server/server-process.ts | 32 |
src/compiler/prerender/prerender-main.ts | 22 |
src/testing/puppeteer/puppeteer-element.ts | 22 |
src/runtime/client-hydrate.ts | 20 |
src/screenshot/connector-base.ts | 19 |
src/runtime/vdom/vdom-render.ts | 17 |
src/compiler/config/test/validate-paths.spec.ts | 16 |
src/dev-server/request-handler.ts | 15 |
src/compiler/prerender/prerender-optimize.ts | 14 |
src/compiler/sys/stencil-sys.ts | 14 |
src/compiler/transpile/transpile-module.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/connected-callback.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 |
Our most common errors
Typescript Error Code | Count |
---|---|
TS2345 | 367 |
TS2322 | 364 |
TS18048 | 223 |
TS18047 | 99 |
TS2722 | 37 |
TS2532 | 26 |
TS2531 | 23 |
TS2454 | 14 |
TS2352 | 12 |
TS2790 | 11 |
TS2769 | 8 |
TS2538 | 8 |
TS2344 | 6 |
TS2416 | 6 |
TS2493 | 3 |
TS18046 | 2 |
TS2684 | 1 |
TS2430 | 1 |
Unused exports report
There are 14 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 | 269 | normalize |
src/utils/index.ts | 7 | escapeRegExpSpecialCharacters |
src/compiler/app-core/app-data.ts | 25 | BUILD |
src/compiler/app-core/app-data.ts | 115 | Env |
src/compiler/app-core/app-data.ts | 117 | 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 |
ea55603
to
0cf181a
Compare
deprecate the `nodeResolve#customResolveOptions` configuration option in a Stencil configuration. this field is used to configure `@rollup/plugin-node-resolve` when generating any non-documentation output target artifacts. the contents of this option get propagated by the rollup plugin to `resolve`, which does the actual resolution of files in stencil's in-memory filesystem. in newer versions of `@rollup/plugin-node-resolve`, this option has been removed, which no option to override defaults (creating a separation of concerns between the plugin and `resolve`). since stencil exposes this configuration in its public api, we cannot remove it without causing a breaking change (nor could we find a suitable way to reuse an existing configuration). this change is a prerequisite to upgrading `@rollup/plugin-node-resolve`. we will not remove the configuration option/upgrade the plugin until stencil v5. in the interim, we add a warning message at config validation time to try to elicit feedback on this change STENCIL-595: Update Rollup v2.X Infrastructure
0cf181a
to
8435748
Compare
type: 'build', | ||
}); | ||
// TODO(STENCIL-1107): Remove this assertion | ||
expect(diagnostics[1]).toEqual({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add this as this field is being used by our tests still. In adding the warning here, that caused an additional warning to trigger.
I figured it'd be best to just:
- Increment the count of warns we assert against for now, since there's no other good way to 'split up' the number of assertions we get
- Add explicit assertions for the existing warning and here. That way, in STENCIL-1107, we can leave the first one and we're a little better off in this test suite than where we started
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
this is the highest version we can upgrade stencil to due to cross library dependencies. v2.57 has a bug in it that requires `@rollup/plugin-commonjs@22`. that in turn, requires `@rollup/plugin-node-resolve@13`. however, `@rollup/plugin-node-resolve@11` has changes that would be breaking for existing users (see #5269)
upgrade rollup to v2.56.3 and `@rollup/plugin-commonjs` to v21.1. this commit raises these libraries to the highest version as possible without introducing breaking changes. v2.57 has a bug in it that requires `@rollup/plugin-commonjs@22` for handling CJS-ESM interop. that version of `@rollup/plugin-commonjs` requires `@rollup/plugin-node-resolve@13`. however, `@rollup/plugin-node-resolve@11` has changes that would be breaking for existing users (see #5269) future upgrades will be slated to stencil v5.0.0. STENCIL-595
upgrade rollup to v2.56.3 and `@rollup/plugin-commonjs` to v21.1. this commit raises these libraries to the highest version as possible without introducing breaking changes. v2.57 has a bug in it that requires `@rollup/plugin-commonjs@22` for handling CJS-ESM interop. that version of `@rollup/plugin-commonjs` requires `@rollup/plugin-node-resolve@13`. however, `@rollup/plugin-node-resolve@11` has changes that would be breaking for existing users (see #5269) in these various versions of both libraries, new flags have been added to configure its output. for now, we continue to use the existing configurations to prevent breaking users. these new configurations have been recorded and tackled when we upgrade to newer versions of rollup (as a part of stencil v5). future upgrades will be slated to stencil v5.0.0. STENCIL-595
upgrade rollup to v2.56.3 and `@rollup/plugin-commonjs` to v21.1. this commit raises these libraries to the highest version as possible without introducing breaking changes. v2.57 has a bug in it that requires `@rollup/plugin-commonjs@22` for handling CJS-ESM interop. that version of `@rollup/plugin-commonjs` requires `@rollup/plugin-node-resolve@13`. however, `@rollup/plugin-node-resolve@11` has changes that would be breaking for existing users (see #5269) in these various versions of both libraries, new flags have been added to configure its output. for now, we continue to use the existing configurations to prevent breaking users. these new configurations have been recorded and tackled when we upgrade to newer versions of rollup (as a part of stencil v5). future upgrades will be slated to stencil v5.0.0. STENCIL-595
This patch has been released as a part of today's Stencil v4.11.0 release. |
What is the current behavior?
we have a stencil configuration option that is part of our public configuration api,
nodeResolve#customResolveOptions
. This option will no longer be honored by newer options of@rollup/plugin-node-resolve
- we need to remove it in the future (a breaking change). for now, we can start the process by logging a warning.GitHub Issue Number: N/A
What is the new behavior?
deprecate the
nodeResolve#customResolveOptions
configuration option in a Stencil configuration. this field is used to configure@rollup/plugin-node-resolve
when generating any non-documentation output target artifacts. the contents of this option get propagated by the rollup plugin toresolve
, which does the actual resolution of files in stencil's in-memory filesystem.in newer versions of
@rollup/plugin-node-resolve
, this option has been removed, which no option to override defaults (creating a separation of concerns between the plugin andresolve
). since stencil exposes this configuration in its public api, we cannot remove it without causing a breaking change (nor could we find a suitable way to reuse an existing configuration).this change is a prerequisite to upgrading
@rollup/plugin-node-resolve
. we will not remove the configuration option/upgrade the plugin until stencil v5. in the interim, we add a warning message at config validation time to try to elicit feedback on this changeDoes this introduce a breaking change?
Testing
npm init stencil@latest component
npm i @stencil/[email protected]
stencil.config.ts
file, add the top level configurationnodeResolve: customRollupOptions: {}
. Observe a deprecation notice.testing: { browserHeadless: "new", }, + nodeResolve: {customResolveOptions: {}}
4. Build the project -
npm run build
. Observe the following warning:Note: The build will fail since we're providing empty options to
resolve
5. Revert the changes to
stencil.config.ts
testing: { browserHeadless: "new", }, - nodeResolve: {customResolveOptions: {}}
npm run build
. Observe the warning is gone.Other information
i've marked this as a 'feat' to raise awareness to it (i want it surfaced in the changelog). if you think 'fix' is more appropriate, happy to change it.