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

Standalone build #257

Closed
wants to merge 11 commits into from
Closed

Standalone build #257

wants to merge 11 commits into from

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Oct 20, 2021

See #239 for more

@dummdidumm
Copy link
Member Author

dummdidumm commented Nov 14, 2021

This works as standalone now when using it with a bundler like Rollup. But it doesn't work yet for a true browser import because there's a prettier and svelte/compiler import inside the standalone ESM. These need to get out of there somehow, because imports like that only work with import maps currently which are not standard yet and only present in chromium based browsers. I guess true standalone browser-ready today would mean bundling to UMD or sth like that.

@raymondmuller
Copy link

@dummdidumm Can this get merged (once conflicts are resolved)?
Even though it doesn't work 'yet' as a true browser import due to the dependencies, you mention it does work when bundled, which IMO is a great addition already (and I could use this now 😄 )

@@ -3,6 +3,7 @@
## 2.5.0 (Unreleased)

* (feat) Support `bracketSameLine` option and deprecate `svelteBracketNewLine` ([#251](https://github.com/sveltejs/prettier-plugin-svelte/pull/251))
* (feat) Add a CJS/ESM standalone build, enables usage inside the browser ([#69](https://github.com/sveltejs/prettier-plugin-svelte/pull/69))
Copy link
Member

Choose a reason for hiding this comment

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

this should possibly be a breaking change since you're adding exports? could be better to make it part of 3.0

"import": "./esm/standalone.mjs",
"default": "./standalone.js"
},
"./package.json": "./package.json"
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 we updated rollup-plugin-svelte, so there's no reason to export this anymore

@@ -1,14 +1,63 @@
import resolve from '@rollup/plugin-node-resolve';
import commonjs from '@rollup/plugin-commonjs';
import typescript from 'rollup-plugin-typescript';
import alias from '@rollup/plugin-alias';
Copy link
Member

Choose a reason for hiding this comment

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

this should probably go before the others to be alphabetized?

@@ -1,14 +1,63 @@
import resolve from '@rollup/plugin-node-resolve';
import commonjs from '@rollup/plugin-commonjs';
import typescript from 'rollup-plugin-typescript';
import alias from '@rollup/plugin-alias';
import path from 'path';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import path from 'path';
import path from 'node:path';

file: 'plugin.js',
format: 'cjs',
sourcemap: true,
const srcDir = path.resolve(__dirname, 'src');
Copy link
Member

Choose a reason for hiding this comment

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

do we want to use svelte naming convetions?

Suggested change
const srcDir = path.resolve(__dirname, 'src');
const src_dir = path.resolve(__dirname, 'src');

@@ -25,7 +26,8 @@ export const parsers: Record<string, Parser> = {
svelte: {
parse: (text) => {
try {
return <ASTNode>{ ...require(`svelte/compiler`).parse(text), __isRoot: true };
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

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

should add a comment why this is necessary. if unfixable should use @ts-expect-error

@curran curran mentioned this pull request Jan 27, 2024
@curran
Copy link
Contributor

curran commented Jan 29, 2024

See also #417

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants