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

Fix import map panics, use import map's location as its base URL #2770

Merged
merged 5 commits into from Aug 15, 2019

Conversation

@nayeemrmn
Copy link
Contributor

commented Aug 12, 2019

Fixes #2769

Panics when

Also:

  • Use import map's location as its base URL.
  • Add tests.

@nayeemrmn nayeemrmn force-pushed the nayeemrmn:import-map-panics branch 3 times, most recently from 2c68b65 to e683a78 Aug 12, 2019

cli/state.rs Outdated Show resolved Hide resolved
cli/import_map.rs Outdated Show resolved Hide resolved
cli/state.rs Outdated Show resolved Hide resolved

@nayeemrmn nayeemrmn force-pushed the nayeemrmn:import-map-panics branch 4 times, most recently from 4704a71 to 828e817 Aug 13, 2019

@ry

This comment has been minimized.

Copy link
Collaborator

commented Aug 13, 2019

Can these panics be demonstrated in the unit tests? Can you give an example of a call that panics currently?

@nayeemrmn

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

@ry

deno --importmap importmap.json

where importmap.json exists, and

deno --importmap importmap.json main.js

where importmap.json does not exist, isn't valid JSON or doesn't meet the requirements of an import map. Regarding rust land tests, I'm not familiar with the architecture but I may be able to figure it out today.

EDIT: I'm not sure any except the first one are unit testable.

@nayeemrmn nayeemrmn changed the title Fix import map panics Fix import map panics, use import map's location as its base URL Aug 13, 2019

@nayeemrmn nayeemrmn force-pushed the nayeemrmn:import-map-panics branch 4 times, most recently from 697dedc to 6814aed Aug 13, 2019

@nayeemrmn

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

@ry Done.

As you suggested I scrapped the integration tests. I've added a unit test each for the first 2 cases, but the third case was fixed by nicely exiting on a ThreadSafeState::new() error in main.rs. That one has a broader scope and I'm not sure how to approach it without an integration test, so I'll leave it to someone else.

@ry
Copy link
Collaborator

left a comment

Awesome - much nicer now.

Just one comment ...

tools/integration_tests.py Outdated Show resolved Hide resolved

@nayeemrmn nayeemrmn force-pushed the nayeemrmn:import-map-panics branch from 23e2a58 to 22d7c0c Aug 15, 2019

@ry
ry approved these changes Aug 15, 2019
Copy link
Collaborator

left a comment

Nicely done - LGTM

@ry ry merged commit 52a66c2 into denoland:master Aug 15, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@nayeemrmn nayeemrmn deleted the nayeemrmn:import-map-panics branch Aug 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.