-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
chore: unbundled esm #8613
Changes from 2 commits
ec3ebaa
391795f
c1492dd
bec6ee1
c1b5aca
e1b67bd
474a4b2
c8f98b5
dbf29eb
e1f6676
2c738b0
55bb947
2473946
f35e998
35ea95b
6aa3e46
eaef8f1
4dbee70
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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}';` | ||
); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the build is run as part of the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
const runtime_entrypoints = Object.fromEntries( | ||
fs | ||
.readdirSync('src/runtime', { withFileTypes: true }) | ||
|
@@ -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', | ||
|
@@ -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) { | ||
|
@@ -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({ | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't we still have sourcemaps for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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') | ||
} | ||
]; |
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.
can we add a comment explaining why this needs to be ignored
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.
The
!
means it's not ignored, which it was previously 😄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.
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