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

Pinning should just always download #217

Merged
merged 1 commit into from
Jan 1, 2024
Merged

Pinning should just always download #217

merged 1 commit into from
Jan 1, 2024

Conversation

dhh
Copy link
Member

@dhh dhh commented Jan 1, 2024

The idea of depending on a CDN to serve assets doesn't actually make sense in a HTTP/2 world. You pay a price per domain you connect to, so you should just serve all your assets, including npm dependencies, off your own domain.

We originally went with CDNs as default out of a fear that people would be squeamish about vendoring compiled libraries together with their source code. But that's just silly. Static linking has a long, proud history, and this is the equivalent of that.

@mhenrixon
Copy link

While I agree with the reasoning, this broke a lot of imports for me that can't even be downloaded (see #230, for example). It may be because some packages are loading an index.js file, which no longer works.

It was released as a major version, but it's still sad to see it break like this. I'll go ahead and downgrade for now.

You are like me; you move a little too fast sometimes @dhh 🙃 I love the progress, though; keep pushing the boundaries of web apps!

@KonnorRogers
Copy link
Contributor

KonnorRogers commented Jan 8, 2024

@mhenrixon this may have been a bug that's always been present, but is just now coming to light. My best guess were CDNs were properly inferring the entrypoint, but for some reason the vendored pins aren't reading the main / module entrypoint in the package.json, not sure if it's a bug in the importmaps-rails implementation, or with JSPM's API.

@KonnorRogers
Copy link
Contributor

Did some digging, and this appears to be expected behavior for vendoring. CDNs work since they already have the package subpaths and can directly link to files. When vendoring, it only vendors the entrypoint you ask for.

So it's largely expected behavior, but perhaps importmaps-rails could smooth this over, particularly for packages that may have potentially many subpath exports.

More here: #230 (comment)

@Caleb-T-Owens
Copy link
Contributor

Hi! I think there are some good reasons for vendoring your JS, but given some of the teething problems that this seems to have caused, I wonder if it would make sense to keep the default of vendoring and introduce a new CLI argument to optionally pin via a CDN.

Would appreciate any thoughts 👍

@dhh
Copy link
Member Author

dhh commented Jan 23, 2024

I’d prefer to just try to solve the issues. JSPM has added a new API just for that purpose: https://jspm.org/cdn/api#download. Please do investigate.

@mhenrixon
Copy link

I’d prefer to just try to solve the issues.

I agree, I love the vendoring. It also makes development possible without internet which I often need.

Let's fix it instead.

@mhenrixon
Copy link

Has anyone started a PR yet? If not I'll give it a go tonight when the kids are sleeping.

@Caleb-T-Owens
Copy link
Contributor

Caleb-T-Owens commented Jan 23, 2024

@mhenrixon I've started to take a look into what's involved, but happy to give up the reigns if you want to have a go.

Edit: I've now started to make some progress on this

@mhenrixon
Copy link

Edit: I've now started to make some progress on this

I'll let you at it, let me know if I'm needed.

@Caleb-T-Owens
Copy link
Contributor

Caleb-T-Owens commented Jan 23, 2024

I have a working prototype with tests written, I'm going to do some testing in a rails app tomorrow with a scenario I ran into in a real project, to verify that that case has been fixed.

@mhenrixon
Copy link

I have a working prototype with tests written, I'm going to do some testing in a rails app tomorrow with a scenario I ran into in a real project, to verify that that case has been fixed.

That's awesome ✨ can't wait to try it out.

This issue made me question my dependencies, and I got rid of everything depending on lit.

I'd love to add back ninja keys, though.

miharekar added a commit to miharekar/visualizer that referenced this pull request Mar 30, 2024
Skypack seems abandoned:
- skypackjs/skypack-cdn#365
- skypackjs/skypack-cdn#362

importmap-rails switched logic to always download: rails/importmap-rails#217

But it doesn't work for complex bigger packages. There's a WIP PR to address this: rails/importmap-rails#235

So I decided to use the new rails-importmap approach where I can, and switch to jsdelivr where I have to.

I've pinned to major versions and jsdelivr should take care of everything else. I've also updated the rake task to check for major version updates.
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.

4 participants