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

chore: unbundled esm #8613

Merged
merged 18 commits into from
May 24, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
node_modules
*.map
/src/compiler/compile/internal_exports.js
/src/shared/version.js
/compiler.d.ts
/compiler.*js
/index.*js
Expand Down
2 changes: 2 additions & 0 deletions .prettierignore
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@
/site
!/src
src/compiler/compile/internal_exports.js
src/shared/version.js
!/test
/test/**/*.svelte
/test/**/_expected*
/test/**/_actual*
/test/**/expected*
/test/**/_output
/types
!rollup.config.js
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 add a comment explaining why this needs to be ignored

Copy link
Member Author

Choose a reason for hiding this comment

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

The ! means it's not ignored, which it was previously 😄

Copy link
Member

Choose a reason for hiding this comment

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

ah, duh. the format of this file is rather peculiar where it ignores everything in the root directory by default and then only opts files in. I wonder if we could swap that once the CJS version is removed

36 changes: 18 additions & 18 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,52 +26,52 @@
".": {
"types": "./types/runtime/index.d.ts",
"browser": {
"import": "./index.mjs",
"require": "./index.js"
"import": "./index.cjs",
"require": "./index.cjs"
},
"import": "./ssr.mjs",
"require": "./ssr.js"
"import": "./src/runtime/ssr.js",
"require": "./ssr.cjs"
},
"./compiler": {
"types": "./types/compiler/index.d.ts",
"import": "./compiler.mjs",
"require": "./compiler.js"
"import": "./src/compiler/index.js",
"require": "./compiler.cjs"
},
"./action": {
"types": "./types/runtime/action/index.d.ts"
},
"./animate": {
"types": "./types/runtime/animate/index.d.ts",
"import": "./animate/index.mjs",
"require": "./animate/index.js"
"import": "./src/runtime/animate/index.js",
"require": "./animate/index.cjs"
},
"./easing": {
"types": "./types/runtime/easing/index.d.ts",
"import": "./easing/index.mjs",
"require": "./easing/index.js"
"import": "./src/runtime/easing/index.js",
"require": "./easing/index.cjs"
},
"./internal": {
"types": "./types/runtime/internal/index.d.ts",
"import": "./internal/index.mjs",
"require": "./internal/index.js"
"import": "./src/runtime/internal/index.js",
"require": "./internal/index.cjs"
},
"./motion": {
"types": "./types/runtime/motion/index.d.ts",
"import": "./motion/index.mjs",
"require": "./motion/index.js"
"import": "./src/runtime/motion/index.js",
"require": "./motion/index.cjs"
},
"./register": {
"require": "./register.js"
},
"./store": {
"types": "./types/runtime/store/index.d.ts",
"import": "./store/index.mjs",
"require": "./store/index.js"
"import": "./src/runtime/store/index.js",
"require": "./store/index.cjs"
},
"./transition": {
"types": "./types/runtime/transition/index.d.ts",
"import": "./transition/index.mjs",
"require": "./transition/index.js"
"import": "./src/runtime/transition/index.js",
"require": "./transition/index.cjs"
},
"./elements": {
"types": "./elements/index.d.ts"
Expand Down
82 changes: 32 additions & 50 deletions rollup.config.mjs → rollup.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,15 @@ const ts_plugin = is_publish
});

fs.writeFileSync(
`./compiler.d.ts`,
'./compiler.d.ts',
`export { compile, parse, preprocess, walk, VERSION } from './types/compiler/index.js';`
);

fs.writeFileSync(
'./src/shared/version.js',
`/** @type {string} */\nexport const VERSION = '${pkg.version}';`
);

Copy link
Contributor

Choose a reason for hiding this comment

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

This would mean one has to run build before doing anything else, eg. running tests. How about keeping it tracked in git and regenerating it in the release action instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since the build is run as part of the prepare script, this will be missing only those who already have the repository checked out - doing pnpm build once is fine for me in that case. It's the same for the internal_exports.js btw which exists for a long time already. I wouldn't want those things to be checked in personally.

Copy link
Member

Choose a reason for hiding this comment

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

we discussed about the difficulties of adding a version file with changeset workflow here sveltejs/kit#9969 as long as it is part of the revision that gets tagged as that version and we are 300% sure the two versions in package.json and version.js can never get out of sync i'd be fine with it, but not without.
A unit test could do this.

Copy link
Member Author

Choose a reason for hiding this comment

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

preblishOnly triggers the build which contains the version.js generation as part of it, at which point the pkg.version has already been updated, so it's safe to do it that way.

Copy link
Member

Choose a reason for hiding this comment

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

personally I don't think the stuff the version is used for is that important and I'd be fine to just drop it as it doesn't really seem necessary and people can read or import the package.json to get the version themselves if they really care about it

const runtime_entrypoints = Object.fromEntries(
fs
.readdirSync('src/runtime', { withFileTypes: true })
Expand All @@ -39,50 +44,37 @@ const runtime_entrypoints = Object.fromEntries(
* @type {import("rollup").RollupOptions[]}
*/
export default [
// Runtime: Generates CJS builds and type definition entry points for svelte/* and generated internal_exports.js
{
input: {
...runtime_entrypoints,
index: 'src/runtime/index.js',
ssr: 'src/runtime/ssr.js'
},
output: ['es', 'cjs'].map(
/** @returns {import('rollup').OutputOptions} */
(format) => {
const ext = format === 'es' ? 'mjs' : 'js';
return {
entryFileNames: (entry) => {
if (entry.isEntry) {
if (entry.name === 'index') return `index.${ext}`;
else if (entry.name === 'ssr') return `ssr.${ext}`;
output: {
entryFileNames: (entry) => {
if (entry.isEntry) {
if (entry.name === 'index') return `index.cjs`;
else if (entry.name === 'ssr') return `ssr.cjs`;

return `${entry.name}/index.${ext}`;
}
},
chunkFileNames: `internal/[name]-[hash].${ext}`,
format,
minifyInternalExports: false,
dir: '.',
};
}
),
return `${entry.name}/index.cjs`;
}
},
chunkFileNames: `internal/[name]-[hash].cjs`,
format: 'cjs',
minifyInternalExports: false,
dir: '.'
benmccann marked this conversation as resolved.
Show resolved Hide resolved
},
plugins: [
replace({
preventAssignment: true,
values: {
__VERSION__: pkg.version,
},
}),
ts_plugin,
{
writeBundle(options, bundle) {
if (options.format !== 'es') return;

writeBundle(_options, bundle) {
for (const entry of Object.values(bundle)) {
const dir = entry.name;
if (!entry.isEntry || !runtime_entrypoints[dir]) continue;

if (dir === 'internal') {
const mod = bundle[`internal/index.mjs`];
const mod = bundle[`internal/index.cjs`];
if (mod) {
fs.writeFileSync(
'src/compiler/compile/internal_exports.js',
Expand All @@ -101,16 +93,15 @@ export default [
}
]
},
/* compiler.js */
// Compiler: Generated CJS/UMD build for the compiler
dummdidumm marked this conversation as resolved.
Show resolved Hide resolved
{
input: 'src/compiler/index.js',
plugins: [
replace({
preventAssignment: true,
values: {
__VERSION__: pkg.version,
'process.env.NODE_DEBUG': false // appears inside the util package
},
}
}),
{
resolveId(id) {
Expand All @@ -119,7 +110,7 @@ export default [
if (id === 'util') {
return require.resolve('./node_modules/util'); // just 'utils' would resolve this to the built-in module
}
},
}
},
resolve(),
commonjs({
Expand All @@ -128,23 +119,14 @@ export default [
json(),
ts_plugin
],
output: [
{
file: 'compiler.js',
format: is_publish ? 'umd' : 'cjs',
name: 'svelte',
sourcemap: true,
},
{
file: 'compiler.mjs',
format: 'esm',
name: 'svelte',
sourcemap: true,
}
],
output: {
file: 'compiler.cjs',
format: is_publish ? 'umd' : 'cjs',
name: 'svelte',
sourcemap: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we still have sourcemaps for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

We decided to remove it because rarely any tool depends on it, and those who do can still backtrack manually

},
external: is_publish
? []
: (id) =>
id === 'acorn' || id === 'magic-string' || id.startsWith('css-tree')
: (id) => id === 'acorn' || id === 'magic-string' || id.startsWith('css-tree')
}
];
3 changes: 2 additions & 1 deletion src/compiler/compile/Component.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
extract_svelte_ignore_from_comments
} from '../utils/extract_svelte_ignore.js';
import check_enable_sourcemap from './utils/check_enable_sourcemap.js';
import { VERSION } from '../../shared/version.js';

const regex_leading_directory_separator = /^[/\\]/;
const regex_starts_with_term_export = /^Export/;
Expand Down Expand Up @@ -320,7 +321,7 @@ export default class Component {
if (result) {
const { compile_options, name } = this;
const { format = 'esm' } = compile_options;
const banner = `${this.file ? `${this.file} ` : ''}generated by Svelte v${'__VERSION__'}`;
const banner = `${this.file ? `${this.file} ` : ''}generated by Svelte v${VERSION}`;

/** @type {any} */
const program = { type: 'Program', body: result.js };
Expand Down
4 changes: 1 addition & 3 deletions src/compiler/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,4 @@ export { default as compile } from './compile/index.js';
export { default as parse } from './parse/index.js';
export { default as preprocess } from './preprocess/index.js';
export { walk } from 'estree-walker';

/** @type {string} */
export const VERSION = '__VERSION__';
export { VERSION } from '../shared/version.js';
5 changes: 2 additions & 3 deletions src/runtime/internal/dev.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
} from './dom.js';
import { SvelteComponent } from './Component.js';
import { is_void } from '../../shared/utils/names.js';
import { VERSION } from '../../shared/version.js';
import { contenteditable_truthy_values } from './utils.js';

/**
Expand All @@ -19,9 +20,7 @@ import { contenteditable_truthy_values } from './utils.js';
* @returns {void}
*/
export function dispatch_dev(type, detail) {
document.dispatchEvent(
custom_event(type, { version: '__VERSION__', ...detail }, { bubbles: true })
);
document.dispatchEvent(custom_event(type, { version: VERSION, ...detail }, { bubbles: true }));
}

/**
Expand Down
5 changes: 4 additions & 1 deletion test/js/js-output.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,10 @@ describe('js-output', () => {

actual = svelte
.compile(input, options)
.js.code.replace(/generated by Svelte v__VERSION__/, 'generated by Svelte vX.Y.Z');
.js.code.replace(
/generated by Svelte v\d+\.\d+\.\d+(-\w+\.\d+)?/,
'generated by Svelte vX.Y.Z'
);
} catch (err) {
console.log(err.frame);
throw err;
Expand Down