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

chore: unbundled esm #8613

merged 18 commits into from May 24, 2023

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented May 22, 2023

BREAKING CHANGE cjs support removed

Svelte no longer supports CJS as a compiler output and its runtime version of CJS and the register hook is removed, too. Use a bundler like Vite or our full-stack framework SvelteKit instead.

Changes

  • remove esm bundle step
  • introduce generated version.js because we can no longer use replace because we don't bundle esm
  • remove register hook, cjs compiler output and cjs runtime
  • keep umd compiler version for prettier/eslint/browser but without sourcemaps
  • move devdependencies to dependencies where necessary
  • various cleanup

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with npm test and lint the project with npm run lint

benmccann and others added 2 commits May 8, 2023 14:51
- removes esm bundle step
- introduces generated version.js because we can no longer use replace because we don't bundle esm
@dummdidumm dummdidumm added this to the 4.x milestone May 22, 2023
@dummdidumm dummdidumm changed the title Unbundled esm chore: unbundled esm May 22, 2023
@vercel
Copy link

vercel bot commented May 22, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
svelte-dev-2 ❌ Failed (Inspect) May 24, 2023 1:17pm

@dummdidumm dummdidumm changed the base branch from master to version-4 May 22, 2023 13:25
@svelte-ecoystem-ci
Copy link

svelte-ecoystem-ci bot commented May 22, 2023

📝 Ran ecosystem CI: Open

suite result
eslint-plugin-svelte ⏹️ cancelled
language-tools ❌ failure
prettier-plugin-svelte ⏹️ cancelled
rollup-plugin-svelte ⏹️ cancelled
svelte-loader ⏹️ cancelled
svelte-preprocess ⏹️ cancelled
sveltekit ⏹️ cancelled
vite-plugin-svelte ⏹️ cancelled

rollup.config.js Outdated
Comment on lines 31 to 35
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

rollup.config.js Outdated Show resolved Hide resolved
!/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

rollup.config.js Outdated Show resolved Hide resolved
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

@dummdidumm
Copy link
Member Author

Talked to Rich about this - decision:

  • we will keep the compiler umd build, but without source maps
  • we get rid of the CJS compiler option and runtime output. We'll see if anyone cries out loud about it and then reasses

@dummdidumm
Copy link
Member Author

/ecosystem-ci run

@svelte-ecoystem-ci
Copy link

svelte-ecoystem-ci bot commented May 22, 2023

📝 Ran ecosystem CI: Open

suite result
eslint-plugin-svelte ❌ failure
language-tools ❌ failure
prettier-plugin-svelte ❌ failure
rollup-plugin-svelte ❌ failure
svelte-loader ❌ failure
svelte-preprocess ❌ failure
sveltekit ❌ failure
vite-plugin-svelte ❌ failure

@dummdidumm
Copy link
Member Author

/ecosystem-ci run

@svelte-ecoystem-ci
Copy link

svelte-ecoystem-ci bot commented May 22, 2023

📝 Ran ecosystem CI: Open

suite result
eslint-plugin-svelte ✅ success
language-tools ❌ failure
prettier-plugin-svelte ✅ success
rollup-plugin-svelte ✅ success
svelte-loader ❌ failure
svelte-preprocess ❌ failure
sveltekit ❌ failure
vite-plugin-svelte ❌ failure

@dummdidumm
Copy link
Member Author

/ecosystem-ci run

@svelte-ecoystem-ci
Copy link

svelte-ecoystem-ci bot commented May 22, 2023

📝 Ran ecosystem CI: Open

suite result
eslint-plugin-svelte ✅ success
language-tools ❌ failure
prettier-plugin-svelte ✅ success
rollup-plugin-svelte ✅ success
svelte-loader ✅ success
svelte-preprocess ✅ success
sveltekit ❌ failure
vite-plugin-svelte ❌ failure

@dummdidumm
Copy link
Member Author

@dominikg do you know what's up with the vite ecosystem test failing? Sounds like ESlint has problems resolving the symlinks or sth?

rollup.config.js Outdated Show resolved Hide resolved
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@@ -6,75 +6,67 @@
"module": "index.mjs",
"main": "index",
"files": [
"src",
Copy link
Member

Choose a reason for hiding this comment

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

we should separate the dependencies and devDependencies in this file as was being done in #8523. i'm not sure if you want to try to merge that PR first or copy those changes into this PR

Copy link
Contributor

@gtm-nayan gtm-nayan May 23, 2023

Choose a reason for hiding this comment

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

If copying over the changes, source-map needs to be kept and so does the util polyfill for the UMD bundle.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for catching that. I updated #8523 to address that

@gtm-nayan
Copy link
Contributor

gtm-nayan commented May 24, 2023

Publint:

Warnings:
1. pkg.exports["./register"].require is ./register.js and is written in CJS, but is interpreted as ESM. Consider using the .cjs extension, e.g. ./register.cjs
Errors:
1. pkg.main is index but the file does not exist.
2. pkg.module is index.mjs but the file does not exist.
 ERROR  Command failed with exit code 1: publint

@dominikg
Copy link
Member

Sorry wasn't clear, I meant the failing test run of v-p-s, something about svelte/compiler not being found.

that should go away after v-p-s jsdoc conversion has merged sveltejs/vite-plugin-svelte#655 sveltejs/vite-plugin-svelte#657 , part of that is switching from eslint-plugin-node to its successor eslint-plugin-n.

Comment on lines +13 to +21
"index.d.ts",
"internal.d.ts",
"store.d.ts",
"animate.d.ts",
"transition.d.ts",
"easing.d.ts",
"motion.d.ts",
"action.d.ts",
"elements.d.ts",
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these needed in addition to the exports map because of the moduleResolution thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct

@dummdidumm
Copy link
Member Author

/ecosystem-ci run

@svelte-ecoystem-ci
Copy link

svelte-ecoystem-ci bot commented May 24, 2023

📝 Ran ecosystem CI: Open

suite result
eslint-plugin-svelte ✅ success
language-tools ❌ failure
prettier-plugin-svelte ✅ success
rollup-plugin-svelte ✅ success
svelte-loader ✅ success
svelte-preprocess ✅ success
sveltekit ❌ failure
vite-plugin-svelte ❌ failure

@dummdidumm dummdidumm merged commit a40af4d into version-4 May 24, 2023
7 of 8 checks passed
@dummdidumm dummdidumm deleted the unbundled-esm branch May 24, 2023 13:31
@gtm-nayan gtm-nayan mentioned this pull request Jun 17, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants