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.

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: log warning if svelte field causes difference in resolve #510

Merged
merged 17 commits into from
Apr 21, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/cold-horses-begin.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/vite-plugin-svelte': patch
---

log deprecation warnings for packages that use the "svelte" field to resolve svelte files differently than standard vite resolve
dominikg marked this conversation as resolved.
Show resolved Hide resolved
29 changes: 29 additions & 0 deletions docs/faq.md
Original file line number Diff line number Diff line change
Expand Up @@ -177,3 +177,32 @@ There is no golden rule, but you can follow these recommendations:

This warning only occurs if you use non-default settings in your vite config that can cause problems in combination with prebundleSvelteLibraries.
You should not use prebundleSvelteLibraries during build or for ssr, disable one of the incompatible options to make that warning (and subsequent errors) go away.

<!-- the following header generates an anchor that is used in logging, do not modify!-->

### deprecated "svelte" field
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

per @dummdidumm's point, I'm not sure we need to actually deprecate the svelte field. we just need to stop resolving it before exports and resolve it after instead. that would make the warning trigger less frequently which would be nicer for users

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, I just looked at the code and I think it works assuming the svelte field will be left in place and only changes the order. if we really did want to get rid of it entirely we'd probably have to change the code, but I'd just suggest changing the title here and note below saying it will be removed and instead say the resolve order will change

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think we can recover from a resolve error unless we always call this.resolve in a try/catch not something i want to do long term.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also having 2 accepted ways to do this doesn't feel great. it makes writing new bundler plugins even more challanging. The only point i see in this whole excercise is that at the end of it the svelte field is no longer used at all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's already two ways to resolve JS files with main and exports, so it's just the analogous thing. As long as we prefer exports we can rip out all the code in vite-plugin-svelte and just defer to Vite

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bluwy iirc you changed the behavior of exports + mainfields lately. Is the above still true? If yes we should be able to remove the custom code for resolveViaPackageJsonSvelte even today.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we change the title from deprecated "svelte" field to something more neutral like prefer "svelte" export condition over "svelte" field?

kept the svelte field as neutral instead of remove. We should not encourage them to remove it for now as that could break older tooling that still relies on it

I agree with what you said in that comment and so the word "deprecated" seems too strong to me

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If exports is define, Vite should ignore mainFields (except browser) completely now. Vite also doesn't have a restriction on what the extensions for mainFields could be, so exporting a Svelte component is fine too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dominikg any thoughts about the heading title here? I think it might be the only outstanding item for us to resolve

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dominikg any thoughts about the heading title here? I think it might be the only outstanding item for us to resolve

i have changed the wording to use conflicting resolve results instead of deprecation.


In the past, Svelte recommended using the custom "svelte" field in package.json to allow libraries to point at .svelte source files.
dominikg marked this conversation as resolved.
Show resolved Hide resolved
This field requires a custom implementation to resolve, so you have to use a bundler plugin and this plugin needs to implement resolving.
Since then, node has added support for [conditional exports](https://nodejs.org/api/packages.html#conditional-exports), which have more generic support in bundlers and node itself. So to increase the compatibility with the wider ecosystem and reduce the implementation needs for current and future bundler plugins, it is recommended that packages use the "svelte" exports condition.
dominikg marked this conversation as resolved.
Show resolved Hide resolved

Example:

```diff
// package.json
- "svelte": "src/index.js"
+ "exports": {
+ "./package.json": "./package.json",
bluwy marked this conversation as resolved.
Show resolved Hide resolved
+ "./*": {
+ "svelte": "./src/*",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need to be this...

Suggested change
+ "svelte": "./src/*",
+ "svelte": "./src/*.svelte",

...in order for library/src/Foo.svelte to be importable as library/Foo? Or is that undesirable/ (Would that pattern even work?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ideally it would be "./*.svelte":{"svelte":"./src/*.svelte"} but that didn't work in my testing even though node docs show a similar pattern for *.js

omitting the extension altogether might confuse tools that look for it and may cause problems when there is a foo.js and a Foo.svelte (hello windows)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Rich's suggestion is the correct way to support it. Using "./*.svelte" means they have to import as my-lib/Foo.svelte

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not an expert on building sane exports map for a new custom condition, so definetely up for improvements. Unfortunately i couldn't make the case with .svelte in the condition work, would love for someone to show a working example.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what does an ideal svelte exports condition look like today? just exporting the index.js that lists all .svelte files? Or use a glob too so that deep importing components can work?

I'd love to have a guideline for package authors here. Prebundling has made it less needed to use deep imports but they can still be a valid choice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dummdidumm @Rich-Harris

ideally this faq entry contains an example of a svelte exports map that works for most/all library authors. Once we merge this PR and they start updating, many are going to have to do a breaking release. If we change the recommended setup again later that would be kind of a d*ck move and put extra pressure on the whole ecosystem by introducing multiple major updates.

Any thoughts on how glob exports should be utilized if at all?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've never seen anyone use glob exports in the wild before. It's probably not a good practice. E.g. if you have a main component that uses a few sub-components, you probably don't want to export the sub-components. I think I would change the example to not use glob exports

In terms of index.js vs deep package path, I don't know if we're wanting to make a universal declaration on that yet. You could potentially show both examples and then link to https://github.com/sveltejs/vite-plugin-svelte/blob/main/docs/faq.md#what-is-going-on-with-vite-and-pre-bundling-dependencies for a deeper discussion of what method to choose

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hadn't thought about internal subcomponents. But i do think we have to provide guidance on how to allow deep imports and/or index imports.

svelte-package stopped generating deep exports into package.json so it would be the library authors task to remember to add new components to that. But maybe thats preferable because with glob exports everything matched by the glob is public and that makes it easier to accidentally do a breaking change without realizing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed the glob example and linked to the other faq entry.

+ },
+ ".": {
+ "svelte": "./index.js"
+ }
}
```

> **Support for the svelte field is deprecated and is going to be removed in a future version of vite-plugin-svelte.**
>
> Library authors are highly encouraged to update their packages to the new exports condition as outlined above. Check out
> [svelte-package](https://kit.svelte.dev/docs/packaging) which already supports it.
33 changes: 28 additions & 5 deletions packages/vite-plugin-svelte/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { toRollupError } from './utils/error';
import { saveSvelteMetadata } from './utils/optimizer';
import { svelteInspector } from './ui/inspector/plugin';
import { VitePluginSvelteCache } from './utils/vite-plugin-svelte-cache';
import { FAQ_LINK_DEPRECATED_SVELTE_FIELD } from './utils/constants';

interface PluginAPI {
/**
Expand Down Expand Up @@ -76,7 +77,7 @@ export function svelte(inlineOptions?: Partial<Options>): Plugin[] {
},

async configResolved(config) {
options = resolveOptions(options, config);
options = resolveOptions(options, config, cache);
patchResolvedViteConfig(config, options);
requestParser = buildIdParser(options);
compileSvelte = createCompileSvelte(options);
Expand Down Expand Up @@ -167,13 +168,35 @@ export function svelte(inlineOptions?: Partial<Options>): Plugin[] {
// for ssr, during scanning and non-prebundled, we do it
if (ssr || scan || !isPrebundled) {
try {
const isFirstResolve = !cache.hasResolvedSvelteField(importee, importer);
const resolved = await resolveViaPackageJsonSvelte(importee, importer, cache);
if (resolved) {
log.debug(
`resolveId resolved ${resolved} via package.json svelte field of ${importee}`
if (isFirstResolve && resolved) {
const packageInfo = await cache.getPackageInfo(resolved);
log.debug.once(
`resolveId resolved ${importee} to ${resolved} via package.json svelte field of ${packageInfo.name}@${packageInfo.version}`
);
return resolved;
const logDeprecationWarning = (error?: Error | string) => {
const prefix = `DEPRECATION WARNING: ${packageInfo.name}@${packageInfo.version} package.json has \`"svelte":"${packageInfo.svelte}"\``;
const reason = error
? ' and without it resolve would fail with the following error:'
: ' and vite would resolve differently without it.';
const secondLine = `Packages should use the "svelte" exports condition instead. See ${FAQ_LINK_DEPRECATED_SVELTE_FIELD} for more information.`;
log.warn.once(`${prefix}${reason}\n${secondLine}`, error);
};
try {
const viteResolved = (
await this.resolve(importee, importer, { ...opts, skipSelf: true })
)?.id;
if (resolved !== viteResolved) {
logDeprecationWarning(
`'${importee}' resolved to '${resolved}' but vite would resolve to '${viteResolved}'`
);
}
} catch (e) {
logDeprecationWarning(e);
}
}
return resolved;
} catch (e) {
log.debug.once(
`error trying to resolve ${importee} from ${importer} via package.json svelte field `,
Expand Down
1 change: 1 addition & 0 deletions packages/vite-plugin-svelte/src/utils/compile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ const _createCompileSvelte = (makeHot: Function) => {
compiled.js.code = makeHot({
id: filename,
compiledCode: compiled.js.code,
// @ts-expect-error hot isn't a boolean anymore at this point
hotOptions: { ...options.hot, injectCss: options.hot?.injectCss === true && hasCss },
compiled,
originalCode: code,
Expand Down
3 changes: 3 additions & 0 deletions packages/vite-plugin-svelte/src/utils/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,6 @@ export const SVELTE_HMR_IMPORTS = [
];

export const SVELTE_EXPORT_CONDITIONS = ['svelte'];

export const FAQ_LINK_DEPRECATED_SVELTE_FIELD =
'https://github.com/sveltejs/vite-plugin-svelte/blob/main/docs/faq.md#deprecated-svelte-field';
2 changes: 1 addition & 1 deletion packages/vite-plugin-svelte/src/utils/log.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ function createLogger(level: string): LogFn {
const logFn: LogFn = _log.bind(null, logger) as LogFn;
const logged = new Set<String>();
const once = function (message: string, payload?: any) {
if (logged.has(message)) {
if (!logger.enabled || logged.has(message)) {
return;
}
logged.add(message);
Expand Down
6 changes: 4 additions & 2 deletions packages/vite-plugin-svelte/src/utils/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
import { atLeastSvelte } from './svelte-version';
import { isCommonDepWithoutSvelteField } from './dependencies';
import { VitePluginSvelteStats } from './vite-plugin-svelte-stats';
import { VitePluginSvelteCache } from './vite-plugin-svelte-cache';

// svelte 3.53.0 changed compilerOptions.css from boolean to string | boolen, use string when available
const cssAsString = atLeastSvelte('3.53.0');
Expand Down Expand Up @@ -180,7 +181,8 @@ function mergeConfigs<T>(...configs: (Partial<T> | undefined)[]): T {
// also validates the final config.
export function resolveOptions(
preResolveOptions: PreResolvedOptions,
viteConfig: ResolvedConfig
viteConfig: ResolvedConfig,
cache: VitePluginSvelteCache
): ResolvedOptions {
const css = cssAsString
? preResolveOptions.emitCss
Expand Down Expand Up @@ -217,7 +219,7 @@ export function resolveOptions(
const statsEnabled =
disableCompileStats !== true && disableCompileStats !== (merged.isBuild ? 'build' : 'dev');
if (statsEnabled && isLogLevelInfo) {
merged.stats = new VitePluginSvelteStats();
merged.stats = new VitePluginSvelteStats(cache);
}
return merged;
}
Expand Down
55 changes: 55 additions & 0 deletions packages/vite-plugin-svelte/src/utils/vite-plugin-svelte-cache.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,17 @@
import { SvelteRequest } from './id';
import { Code, CompileData } from './compile';
import { readFileSync } from 'fs';
import { dirname } from 'path';
//eslint-disable-next-line node/no-missing-import
import { findClosestPkgJsonPath } from 'vitefu';
import { normalizePath } from 'vite';

interface PackageInfo {
name: string;
version: string;
svelte?: string;
path: string;
}

export class VitePluginSvelteCache {
private _css = new Map<string, Code>();
Expand All @@ -8,6 +20,7 @@ export class VitePluginSvelteCache {
private _dependants = new Map<string, Set<string>>();
private _resolvedSvelteFields = new Map<string, string>();
private _errors = new Map<string, any>();
private _packageInfos: PackageInfo[] = [];

public update(compileData: CompileData) {
this._errors.delete(compileData.normalizedFilename);
Expand Down Expand Up @@ -110,6 +123,9 @@ export class VitePluginSvelteCache {
return this._resolvedSvelteFields.get(this._getResolvedSvelteFieldKey(name, importer));
}

public hasResolvedSvelteField(name: string, importer?: string) {
return this._resolvedSvelteFields.has(this._getResolvedSvelteFieldKey(name, importer));
}
public setResolvedSvelteField(
importee: string,
importer: string | undefined = undefined,
Expand All @@ -124,4 +140,43 @@ export class VitePluginSvelteCache {
private _getResolvedSvelteFieldKey(importee: string, importer?: string): string {
return importer ? `${importer} > ${importee}` : importee;
}

public async getPackageInfo(file: string): Promise<PackageInfo> {
let info = this._packageInfos.find((pi) => file.startsWith(pi.path));
if (!info) {
info = await findPackageInfo(file);
this._packageInfos.push(info);
}
return info;
}
}

/**
* utility to get some info from the package.json a file belongs to (closest one with a "name" set)
dominikg marked this conversation as resolved.
Show resolved Hide resolved
*
* @param {string} file to find info for
* @returns {PackageInfo}
*/
async function findPackageInfo(file: string): Promise<PackageInfo> {
const info: PackageInfo = {
name: '$unknown',
version: '0.0.0-unknown',
path: '$unknown'
};
let path = await findClosestPkgJsonPath(file, (pkgPath) => {
const pkg = JSON.parse(readFileSync(pkgPath, 'utf-8'));
if (pkg.name != null) {
info.name = pkg.name;
if (pkg.version != null) {
info.version = pkg.version;
}
info.svelte = pkg.svelte;
return true;
}
return false;
});
// return normalized path with appended '/' so .startsWith works for future file checks
path = normalizePath(dirname(path ?? file)) + '/';
info.path = path;
return info;
}
38 changes: 6 additions & 32 deletions packages/vite-plugin-svelte/src/utils/vite-plugin-svelte-stats.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
import { log } from './log';
//eslint-disable-next-line node/no-missing-import
import { findClosestPkgJsonPath } from 'vitefu';
import { readFileSync } from 'fs';
import { dirname } from 'path';
import { performance } from 'perf_hooks';
import { normalizePath } from 'vite';
import { VitePluginSvelteCache } from './vite-plugin-svelte-cache';

interface Stat {
file: string;
Expand Down Expand Up @@ -87,31 +84,13 @@ function formatPackageStats(pkgStats: PackageStats[]) {
return table;
}

/**
* utility to get the package name a file belongs to
*
* @param {string} file to find package for
* @returns {path:string,name:string} tuple of path,name where name is the parsed package name and path is the normalized path to it
*/
async function getClosestNamedPackage(file: string): Promise<{ name: string; path: string }> {
let name = '$unknown';
let path = await findClosestPkgJsonPath(file, (pkgPath) => {
const pkg = JSON.parse(readFileSync(pkgPath, 'utf-8'));
if (pkg.name != null) {
name = pkg.name;
return true;
}
return false;
});
// return normalized path with appended '/' so .startsWith works for future file checks
path = normalizePath(dirname(path ?? file)) + '/';
return { name, path };
}

export class VitePluginSvelteStats {
// package directory -> package name
private _packages: { path: string; name: string }[] = [];
private _cache: VitePluginSvelteCache;
private _collections: StatCollection[] = [];
constructor(cache: VitePluginSvelteCache) {
this._cache = cache;
}
startCollection(name: string, opts?: Partial<CollectionOptions>) {
const options = {
...defaultCollectionOptions,
Expand Down Expand Up @@ -186,12 +165,7 @@ export class VitePluginSvelteStats {
private async _aggregateStatsResult(collection: StatCollection) {
const stats = collection.stats;
for (const stat of stats) {
let pkg = this._packages.find((p) => stat.file.startsWith(p.path));
if (!pkg) {
pkg = await getClosestNamedPackage(stat.file);
this._packages.push(pkg);
}
stat.pkg = pkg.name;
stat.pkg = (await this._cache.getPackageInfo(stat.file)).name;
}

// group stats
Expand Down