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

SvelteKit / Vite compatibility #144

Closed
wants to merge 1 commit into from
Closed

Conversation

benmccann
Copy link

Closes #143

@lgarron
Copy link
Owner

lgarron commented Oct 15, 2021

Thanks for the suggested fix!

I have a strong policy of using type: "module" instead of .mjs files whenever possible, but this might be a reasonable exception because the main point of this library is now backwards compatibility (so there are issues with "type": "module" at the top-level package.json).

But I'd still really like to avoid the .mjs extension if at all possible. :-/
Do you know if there's a workaround, such as building the ESM targets into a folder like dist/esm and putting a package.json containing "type": "module" in just that folder?

@benmccann
Copy link
Author

I would be totally fine with "type": "module" instead. I tested that in my project and it works for me

Putting a nested package.json sounds a little scary to me. I've never seen that done before and am not totally sure all the implications of it. I agree the .esm.js file extension looks nicer than .mjs, but it's all hidden from the end user, so I figured it wasn't a big deal to change it

@lgarron
Copy link
Owner

lgarron commented Oct 15, 2021

I would be totally fine with "type": "module" instead. I tested that in my project and it works for me

Putting a nested package.json sounds a little scary to me. I've never seen that done before and am not totally sure all the implications of it.

I do this all the time, mostly to work around node "quirks", which a lot of tools seem to match.

I think you've convinced me that adding .mjs files is alright. But I'm reluctant remove any .esm.js artifacts, since those files are meant to be self-contained so you can e.g. drop them right into a static server — .mjs may not work for the kinds of servers that need this library, and I'd like to avoid renaming the code-containing files in case anyone's used to replacing them by hand (and expects the name to stay the same).

Do you know if it's sufficient if the entry point is .mjs? I think I'd be fairly comfortable with an .mjs file next to each of these targets containing export * from "./___.esm.js";.

@lgarron
Copy link
Owner

lgarron commented Oct 15, 2021

Is there a minimal dependent repro I use to test out a few possible options?

@benmccann
Copy link
Author

It's not exactly a minimal repro, but should hopefully still get the job done. Here's the project where I encountered this:

git clone [email protected]:benmccann/attractions.git
cd attractions/docs
git checkout reproduction
npm install
npm run build

@lgarron
Copy link
Owner

lgarron commented Oct 16, 2021

It's not exactly a minimal repro, but should hopefully still get the job done.

Thanks! Are you sure you pushed the reproduction branch, though? 🤔
Screen Shot 2021-10-16 at 03 05 12

@benmccann
Copy link
Author

Oops! Sorry. It's there now: https://github.com/benmccann/attractions/tree/reproduction

@benmccann
Copy link
Author

@lgarron any thoughts on the best path forward here?

@lgarron
Copy link
Owner

lgarron commented Oct 29, 2021

@lgarron any thoughts on the best path forward here?

I still need to test options against the repro!

@lgarron
Copy link
Owner

lgarron commented Feb 18, 2022

Given that I'm deprecating this library, I will close this for now.

This library has been unnecessary in modern browsers for well over a year, and I'd like to avoid any changes that are not specifically fixed for legacy compatibility.

Thanks for the PR and discussion, though — I still appreciate it!

@lgarron lgarron closed this Feb 18, 2022
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.

Does not work with SvelteKit in production mode
2 participants