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

Make Dino.bundle() work like dino bundle? #6734

Closed
wants to merge 7 commits into from

Conversation

sethp
Copy link

@sethp sethp commented Jul 13, 2020

This admittedly falls at least a little into the "I'm holding it wrong" category of use-cases, but in trying to see what it would be like to turn dino into a quick-and-dirty dev bundler I found that dino bundle can handle two things that Dino.bundle() won't:

  1. Import maps
  2. Dynamic imports

This change would bring the two closer together in terms of capability, if that's desirable.

Currently, `dino bundle` can handle two things that `Dino.bundle()` won't:

1. Import maps
2. Dynamic imports

This change brings the two closer together in terms of capability.
@CLAassistant
Copy link

CLAassistant commented Jul 13, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

sethp added 5 commits July 13, 2020 10:10
Currently getting:

```
The filename, directory name, or volume label syntax is incorrect. (os error 123)
```

Perhaps a file url would help.
Should fix:
```
Uncaught URIError: File URL contains invalid path
```
@bartlomieju bartlomieju added cli related to cli/ dir feat new feature (which has been agreed to/accepted) public API related to "Deno" namespace in JS labels Jul 14, 2020
@sethp
Copy link
Author

sethp commented Jul 14, 2020

Thanks to @rudoi for helping me figure out how to get the Windows test working in 2ed4a1e!

@kitsonk
Copy link
Contributor

kitsonk commented Jul 15, 2020

Ref #4752

I am not sure that we want to inherit the CLI's import-map instead of allowing one to be passed to the bundle. As it stands it is an improvement over the existing behaviour though.

When you say it doesn't support dynamic imports, what do you mean? This PR doesn't seem to address that as all if there is an issue there.

@bartlomieju
Copy link
Member

Ref #4752

I am not sure that we want to inherit the CLI's import-map instead of allowing one to be passed to the bundle. As it stands it is an improvement over the existing behaviour though.

Agreed, that should be passed as an option in 3rd argument.

When you say it doesn't support dynamic imports, what do you mean? This PR doesn't seem to address that as all if there is an issue there.

deno bundle analyzes and fetches dynamic imports that can be recognized during compile time. I think it's been changed in this PR, as it's only a single switch on ModuleGraph.

@kitsonk
Copy link
Contributor

kitsonk commented Jul 15, 2020

Agreed, that should be passed as an option in 3rd argument.

Urrr... better to have an options bag... but still better to not take the change until we refactor in #4752 IMO.

deno bundle analyzes and fetches dynamic imports that can be recognized during compile time. I think it's been changed in this PR, as it's only a single switch on ModuleGraph.

👍

@sethp
Copy link
Author

sethp commented Jul 15, 2020

Sounds like the consensus is:

  1. We'd like to expose options for Deno.bundle() so it could be made to work like deno bundle, but
  2. The runtime compiler APIs are planned to undergo a major redesign, and it doesn't make sense to put too much more into the current API in light of that.

Is that right? If so, it probably makes the most sense to close this PR and continue in #4752.

@bartlomieju
Copy link
Member

@sethp thanks for the work on this PR, but I'm gonna close it without merge. We're in the middle of big compiler refactor in #6894 which will change a lot of APIs. We can revisit the topic afterwards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli related to cli/ dir feat new feature (which has been agreed to/accepted) public API related to "Deno" namespace in JS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants