Skip to content
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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 18, 2024
Merged

feat(compiler): deprecate customResolveOptions config option #5269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion src/compiler/config/test/validate-custom.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,24 @@ describe('validateCustom', () => {
},
];
const { diagnostics } = validateConfig(userConfig, mockLoadConfigInit());
expect(diagnostics.length).toBe(1);
// TODO(STENCIL-1107): Decrement the right-hand side value from 2 to 1
expect(diagnostics.length).toBe(2);
// TODO(STENCIL-1107): Keep this assertion
expect(diagnostics[0]).toEqual({
header: 'Build Warn',
level: 'warn',
lines: [],
messageText: 'test warning',
type: 'build',
});
// TODO(STENCIL-1107): Remove this assertion
expect(diagnostics[1]).toEqual({
Copy link
Contributor Author

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:

  1. 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
  2. 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

header: 'Build Warn',
level: 'warn',
lines: [],
messageText:
'nodeResolve.customResolveOptions is a deprecated option in a Stencil Configuration file. If you need this option, please open a new issue in the Stencil repository (https://github.com/ionic-team/stencil/issues/new/choose)',
type: 'build',
});
});
});
11 changes: 10 additions & 1 deletion src/compiler/config/validate-config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { createNodeLogger, createNodeSys } from '@sys-api-node';
import { buildError, isBoolean, isNumber, isString, sortBy } from '@utils';
import { buildError, buildWarn, isBoolean, isNumber, isString, sortBy } from '@utils';

import {
ConfigBundle,
Expand Down Expand Up @@ -265,6 +265,15 @@ export const validateConfig = (
return arr;
}, [] as RegExp[]);

// TODO(STENCIL-1107): Remove this check. It'll be unneeded (and raise a compilation error when we build Stencil) once
// this property is removed.
if (validatedConfig.nodeResolve?.customResolveOptions) {
const warn = buildWarn(diagnostics);
// this message is particularly long - let the underlying logger implementation take responsibility for breaking it
// up to fit in a terminal window
warn.messageText = `nodeResolve.customResolveOptions is a deprecated option in a Stencil Configuration file. If you need this option, please open a new issue in the Stencil repository (https://github.com/ionic-team/stencil/issues/new/choose)`;
}

CACHED_VALIDATED_CONFIG = validatedConfig;

return {
Expand Down
4 changes: 4 additions & 0 deletions src/declarations/stencil-public-compiler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1613,8 +1613,12 @@ export interface NodeResolveConfig {
only?: Array<string | RegExp>;
modulesOnly?: boolean;

// TODO(STENCIL-1107): Remove this field [BREAKING_CHANGE]
/**
* @see https://github.com/browserify/resolve#resolveid-opts-cb
* @deprecated the `customResolveOptions` field is no longer honored in future versions of
* `@rollup/plugin-node-resolve`. If you are currently using it, please open a new issue in the Stencil repo to
* describe your use case & provide input (https://github.com/ionic-team/stencil/issues/new/choose)
*/
customResolveOptions?: {
basedir?: string;
Expand Down
2 changes: 2 additions & 0 deletions src/testing/mocks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ export function mockConfig(overrides: Partial<UnvalidatedConfig> = {}): Unvalida
minifyJs: false,
namespace: 'Testing',
nodeResolve: {
// TODO(STENCIL-1107): Remove this field - it's currently overriding Stencil's default options to pass into
// the `@rollup/plugin-node-resolve` plugin.
customResolveOptions: {},
},
outputTargets: null,
Expand Down