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

Update default provider to use jspm.io instead of jspm #234

Merged
merged 1 commit into from
Jan 23, 2024

Conversation

josefarias
Copy link
Contributor

@josefarias josefarias commented Jan 15, 2024

Coming from basecamp/local_time#113 (comment)

I realized the HTTP request in Importmap::Packager#import was returning HTTP401:

Code:
401

Body:
{"error":"Error: No provider named \"jspm\" has been defined."}

This matches the behavior of a test that's currently failing:

❯ bin/test test/packager_integration_test.rb

Error:
Importmap::PackagerIntegrationTest#test_successful_import_against_live_service:
NoMethodError: undefined method `[]' for nil:NilClass
    /Users/jose/my_repos/importmap-rails/test/packager_integration_test.rb:8:in `block in <class:PackagerIntegrationTest>'


bin/test /Users/jose/my_repos/importmap-rails/test/packager_integration_test.rb:7

This happens because Importmap::Packager#import returns nil when the HTTP response is 401:

case response.code
when "200" then extract_parsed_imports(response)
when "404", "401" then nil
else handle_failure_response(response)
end

Per https://jspm.org/cdn/api#download, the accepted provider options are jspm.io | jsdelivr | unpkg — So the fix should be as simple as using jspm.io instead of jspm.

All tests pass after this change.

Possibly related: jspm/generator#274

@guybedford
Copy link
Contributor

To chime in here - we updated the main API service for JSPM to the Generator 2.0.0 version last night, which looks like it caused this break. I've just re-added a backwards compat fix here to the API service and deployed that so the test should now again be passing and this PR should no longer be necessary - but it can still be landed since we are exactly using jspm.io as the primary provider name.

@josefarias
Copy link
Contributor Author

To chime in here - we updated the main API service for JSPM to the Generator 2.0.0 version last night, which looks like it caused this break. I've just re-added a backwards compat fix here to the API service and deployed that so the test should now again be passing and this PR should no longer be necessary - but it can still be landed since we are exactly using jspm.io as the primary provider name.

Thanks for the context and quick fix @guybedford. Happy to either move forward or drop this PR. I'll leave it up to the maintainers.

Can confirm things work now without this change per my own local tests.

@dhh
Copy link
Member

dhh commented Jan 23, 2024

@guybedford If you plan to keep the alias, I'd prefer not to merge this, as "jspm.io" is a bit of an awkward CLI parameter compared to just "jspm". But don't want this to break going forward!

@josefarias
Copy link
Contributor Author

@dhh could also do something like 486c5d1

But agree we don't need to be overly defensive if the alias is being kept long term.

@josefarias josefarias force-pushed the update-jsm-provider-name branch from eb38b2d to a2b27ed Compare January 23, 2024 15:06
@guybedford
Copy link
Contributor

In the generator and JSPM CLI itself we only now support jspm.io as the provider value, but for the JSPM API I'm happy to continue to support the alias going forward.

@dhh dhh merged commit ddf9be4 into rails:main Jan 23, 2024
21 checks passed
@dhh
Copy link
Member

dhh commented Jan 23, 2024

Might as well go with your primary name, then. But thanks for maintaining the alias until people have upgraded 🙏

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.

3 participants