sveltestrap icon indicating copy to clipboard operation
sveltestrap copied to clipboard

Fix popperjs import for SvelteKit

Open benmccann opened this issue 2 years ago • 19 comments

Just import it the normal way. Then Vite can use the CJS version on the server-side and ESM version on the client-side. Forcing it to always be ESM breaks SSR with the latest SvelteKit. See https://github.com/vitejs/vite/issues/4569 / https://github.com/sveltejs/kit/issues/2161

benmccann avatar Aug 11 '21 04:08 benmccann

Hi, thanks. Popper was giving troubles in Sapper and svelte.repl without this though due to use of process.env in their default distribution... I'll need to take a look at if this still works there, but if not we'd need some other resolution for SvelteKit.

bestguy avatar Aug 11 '21 14:08 bestguy

Related to this issue: https://github.com/bestguy/sveltestrap/issues/250

bestguy avatar Aug 11 '21 14:08 bestguy

Two wrongs don't make a right :smile: We should fix it in popper core. I sent a PR there: https://github.com/popperjs/popper-core/pull/1342

Sapper's deprecated at this point. In the short-term, I'd rather have it working in SvelteKit and broken in the REPL and Sapper than vice versa since SvelteKit will be the main way people will be consuming the library. Hopefully in the medium-term we can get popper.js itself fixed rather than trying to work around the issue

benmccann avatar Aug 11 '21 17:08 benmccann

Agreed popper would be great to fix, but I and other users use sapper and repl frequently and would rather not break that. I'll see if there's something we can do. I had debated forking popper as well to remove, but eh.

bestguy avatar Aug 11 '21 20:08 bestguy

Agreed popper would be great to fix, but I and other users use sapper and repl frequently and would rather not break that. I'll see if there's something we can do. I had debated forking popper as well to remove, but eh.

Actually SvelteStrap works fine with SvelteKit now if you do import from root (a regular way), no changes needed.

vfilatov avatar Aug 13 '21 15:08 vfilatov

Ah thanks @vfilatov , does sveltekit not need the src imports for SSR such as sapper needed?

bestguy avatar Aug 13 '21 16:08 bestguy

Ah thanks @vfilatov , does sveltekit not need the src imports for SSR such as sapper needed?

After SvelteKit v1.0.0-next.146 no, it does not.

  import {
    Alert,
    Badge,
    Button,

    Modal,
    ModalHeader,
    ModalBody,
    ModalFooter
  } from 'sveltestrap';

vfilatov avatar Aug 13 '21 16:08 vfilatov

Ah, the example I was debugging did import from src. I didn't even notice that. https://github.com/sveltejs/kit/issues/2161#issue-965482733

I do still think this is a good change, but I guess we can wait until popper itself is fixed

In the meantime, I sent a PR to update the readme: https://github.com/bestguy/sveltestrap/pull/359

benmccann avatar Aug 13 '21 17:08 benmccann

also in https://github.com/bestguy/sveltestrap/blob/master/src/Tooltip.svelte#L3 https://github.com/bestguy/sveltestrap/blob/master/src/popper.js#L1

bato3 avatar Mar 25 '22 10:03 bato3

Bump, would love to see this merged!

avafloww avatar Apr 08 '22 03:04 avafloww

@bestguy my PR https://github.com/floating-ui/floating-ui/pull/1342 was merged to fix popper-core. Could this be merged now as a result?

benmccann avatar Apr 13 '22 03:04 benmccann

looks like they reverted it https://github.com/floating-ui/floating-ui/pull/1362 (would have been a breaking change)

is it an option to switch to floatingui dom?

edit: there's another PR trying to add export maps to popper again https://github.com/floating-ui/floating-ui/pull/1526

dominikg avatar Apr 13 '22 06:04 dominikg

It looks like it's been fixed on the main branch for an eventual v3 https://github.com/floating-ui/floating-ui/pull/1502 and that https://github.com/floating-ui/floating-ui/pull/1526 would backport it to 2.x

Anyway, I wonder if the original reason for not merging this still applies. It was said that we wanted to prioritize Sapper over SvelteKit, but that was over a year ago. Now SvelteKit has almost 2x the usage of Sapper and so I would think that it would be better to prioritize SvelteKit over Sapper. Would that logic make sense to you @bestguy? If you're still on Sapper by chance, I'd be happy to advice on a migration to SvelteKit

benmccann avatar Apr 13 '22 23:04 benmccann

Is there any alternative or workaround that can be used before this PR get merged?

Edit: I found that the fork by laxadev/sveltestrap is working fine without any errors. I install its dependencies by running npm i git+https://github.com/laxadev/sveltestrap.git.

ynshung avatar Apr 18 '22 04:04 ynshung

I am experiencing the same issue with Sveltestrap. It actually prevents me from running SvelteKit. I'd really like to use bootstrap for svelte app dev. Any workarounds would be appreciated, and we really need a proper fix to this. Thanks.

Cannot use import statement outside a module /Users/john/projects/dems/node_modules/@popperjs/core/dist/esm/popper.js:1 import { popperGenerator, detectOverflow } from "./createPopper.js"; ^^^^^^

SyntaxError: Cannot use import statement outside a module at Object.compileFunction (node:vm:352:18) at wrapSafe (node:internal/modules/cjs/loader:1032:15) :

With this package.json:

{ "name": "dems", "version": "0.0.1", "scripts": { "dev": "svelte-kit dev", "build": "svelte-kit build", "package": "svelte-kit package", "preview": "svelte-kit preview", "prepare": "svelte-kit sync" }, "devDependencies": { "@sveltejs/adapter-auto": "next", "@sveltejs/kit": "next", "svelte": "^3.47.0" }, "type": "module", "dependencies": { "@fontsource/fira-mono": "^4.5.0", "@lukeed/uuid": "^2.0.0", "bootstrap": "^5.1.3", "cookie": "^0.4.1", "sveltestrap": "^5.9.0" } }

king612 avatar Apr 18 '22 16:04 king612

I use manual add

{
  "name": "@popperjs/core",
  "type": "module", // <<<<<<<<<<<<<
  "version": "2.11.5",

to ./node_modules/.pnpm/node_modules/@popperjs/core/package.json as a temporary workaround

I am experiencing the same issue with Sveltestrap. It actually prevents me from running SvelteKit. I'd really like to use bootstrap for svelte app dev. Any workarounds would be appreciated, and we really need a proper fix to this. Thanks.

Cannot use import statement outside a module /Users/john/projects/dems/node_modules/@popperjs/core/dist/esm/popper.js:1 import { popperGenerator, detectOverflow } from "./createPopper.js"; ^^^^^^

SyntaxError: Cannot use import statement outside a module at Object.compileFunction (node:vm:352:18) at wrapSafe (node:internal/modules/cjs/loader:1032:15) :

With this package.json:

{ "name": "dems", "version": "0.0.1", "scripts": { "dev": "svelte-kit dev", "build": "svelte-kit build", "package": "svelte-kit package", "preview": "svelte-kit preview", "prepare": "svelte-kit sync" }, "devDependencies": { "@sveltejs/adapter-auto": "next", "@sveltejs/kit": "next", "svelte": "^3.47.0" }, "type": "module", "dependencies": { "@fontsource/fira-mono": "^4.5.0", "@lukeed/uuid": "^2.0.0", "bootstrap": "^5.1.3", "cookie": "^0.4.1", "sveltestrap": "^5.9.0" } }

vfilatov avatar Apr 18 '22 17:04 vfilatov

I ran into the same issue. I use SvelteKit (1.0.0-next.338 as of writing) and my project could not start up because of the above detailed issue.

Is there any alternative or workaround that can be used before this PR get merged?

Edit: I found that the fork by laxadev/sveltestrap is working fine without any errors. I install its dependencies by running npm i git+https://github.com/laxadev/sveltestrap.git.

The fork suggested by @ynshung worked flawlessly for me.

skornel02 avatar May 22 '22 07:05 skornel02

I have compiled some of the PRs opened into one + updated the dependencies (patch & minor, not major)

  • the PR : https://github.com/bestguy/sveltestrap/pull/488
  • the corresponding fork : https://github.com/ClapClap-app/sveltestrap/
  • to use it : npm i -D github:ClapClap-app/sveltestrap

As proposed in another issue, we should organize ourselves to maintain an "official" fork :)

kbsali avatar May 30 '22 12:05 kbsali

Everyone, and @benmccann @kbsali , I really appreciate the PRs and desire to fix this for SvelteKit, but none of these fixes and forks changing to '@popperjs/core' correct the root issue, which is why the PRs to change popper imports have not been merged after so long.

I've tried the same in past as I mentioned, but I've been unwilling to break the Svelte REPL. This is not deprecated AFAIK, and I'm a heavy user with Sveltestrap. (I know Sapper is deprecated so I wont worry about that one any longer).

Upgrading to Floating UI won't fix this either, as they continue the process.env usage: https://floating-ui.com/docs/getting-started#package-entry-points , and they have not accepted PRs in past to change this. It's really frustrating but that is their choice.

Only solutions I can think of is:

  • use something other than popper or floating-ui (please suggest if you are aware of libaries)
  • Fork popper and remove process.env and use.
  • Change Svelte REPL rollup config with the workaround popper suggests above. (I can try and PR/issue there, but that repo seems even less responsive than I am! 😅)

I will gladly take any help or ideas that fixes this if it works on svelte.dev
I really do not want to break this!

I'll see if https://github.com/Simonwep/nanopop could be used without breaking changes instead of popper.

bestguy avatar Oct 02 '22 03:10 bestguy

Ah, I hadn't realized the REPL issue. Thanks for clarifying.

I've mentioned the REPL issue to the other Svelte maintainers. I'm of two minds as to whether we should makes changes there. On the one hand, there are some libraries that won't work if we don't polyfill process.env. On the other hand, we should discourage process.env usage. Perhaps there's some third option like an advanced mode to let you edit the Rollup config. It's not a simple case to decide what is ideal. We can certainly review a PR if we decide, but deciding is not easy.

It's not clear to me that floating-ui has rejected replacing process.env. It was the PR author who closed the PR (https://github.com/floating-ui/floating-ui/pull/2029). This would be the best solution, so I think we should pursue it still. I think we just need to do the work to make sure it works in all cases and the maintainers are comfortable that it works in all cases.

In the meantime, usage with either the REPL or SvelteKit is impacted. I'm a bit surprised about picking the REPL over SvelteKit since many more users will be using SvelteKit. Also, there is currently a workaround implemented which hides the true issue contributing to it not being resolved yet. StackBlitz could be used for demos of this library rather than the REPL since there you have access to the Rollup config and can modify it as required.

benmccann avatar Mar 24 '23 18:03 benmccann

I'm making PRs to remove these in both Popper and Floating UI. I don't think they're worth the trouble anymore, especially since the lib is usually used transitively.

atomiks avatar Mar 25 '23 22:03 atomiks

That's great. Once https://github.com/floating-ui/floating-ui/pull/2296 is merged, I can bump this PR to use the latest version of Popper, which should make this library work everywhere

benmccann avatar Mar 26 '23 04:03 benmccann

I'm surprised this is not higher priority. For me, it breaks my first attempt at using sveltestrap altogether.

The "hello world" style example:

<script>
    import { Button } from 'sveltestrap'
</script>
<Button>a bootstrap button</Button>

triggers this error. Here's my package.json:

{
	"name": "svelte-app",
	"version": "0.0.1",
	"private": true,
	"scripts": {
		"dev": "vite dev",
		"build": "vite build",
		"preview": "vite preview",
		"lint": "prettier --plugin-search-dir . --check . && eslint .",
		"format": "prettier --plugin-search-dir . --write ."
	},
	"devDependencies": {
		"@sveltejs/adapter-auto": "^2.0.0",
		"@sveltejs/kit": "^1.5.0",
		"eslint": "^8.28.0",
		"eslint-config-prettier": "^8.5.0",
		"eslint-plugin-svelte3": "^4.0.0",
		"prettier": "^2.8.0",
		"prettier-plugin-svelte": "^2.8.1",
		"svelte": "^3.54.0",
		"vite": "^4.2.0"
	},
	"type": "module",
	"dependencies": {
		"@sveltejs/adapter-static": "^2.0.1",
		"bootstrap": "^5.2.3",
		"sveltestrap": "^5.10.0"
	}
}

ps: this workaround appears to work.

godmar avatar Apr 08 '23 20:04 godmar

@bestguy my PR was merged to Popper and I updated this PR to use the latest version. You should be able to merge this PR now to make sveltestrap work out-of-the-box with both SvelteKit and the REPL

benmccann avatar May 26 '23 18:05 benmccann

Which versions in the devDependencies in package.json still work with Sveltestrap out-of-the-box?

  "devDependencies": {
    "@sveltejs/adapter-auto": "^?",
    "@sveltejs/kit": "^?",
    "@typescript-eslint/eslint-plugin": "^?",
    "@typescript-eslint/parser": "^?",
    "eslint": "^?",
    "eslint-config-prettier": "^?",
    "eslint-plugin-svelte": "^?",
    "prettier": "^?",
    "prettier-plugin-svelte": "^?",
    "svelte": "^?",
    "svelte-check": "^?",
    "tslib": "^?",
    "typescript": "^?",
    "vite": "^?"
  }

MattPhantastic avatar Jul 30 '23 03:07 MattPhantastic

@bestguy are there any concerns you have that are keeping you from merging this?

benmccann avatar Jul 30 '23 04:07 benmccann

@bestguy @gary-mycase just checking in here again. To summarize this uses the latest popper which includes https://github.com/floating-ui/floating-ui/pull/2296, so will now work out-of-the-box in both SvelteKit and the REPL

benmccann avatar Aug 04 '23 17:08 benmccann

Thanks @benmccann , let me test over weekend with a pre-release, will try and get this out asap with the 5.3 release

bestguy avatar Aug 04 '23 23:08 bestguy

Hurray! Thank you so much!

benmccann avatar Aug 05 '23 03:08 benmccann

@bestguy I added one more PR #564

laxadev avatar Aug 06 '23 11:08 laxadev