Support deno target by juzi5201314 · Pull Request #908 · drager/wasm-pack

juzi5201314 · 2020-08-27T05:21:41Z

[x ] You have the latest version of rustfmt installed
[x ] You ran cargo fmt on the code base before submitting
Related issues
I want to build wasm for Deno. #672
Targeting Deno #879

OliverBrotchie · 2020-08-30T03:24:40Z

Is there any update on this? It is mentioned in the docs already here.

juzi5201314 · 2020-08-30T03:31:06Z

As #879 (comment) said, wasm-bingen seems to have recently gained support for deno target, but the option is not forwarded to it by wasm-pack.

OliverBrotchie

The changes are very minor, I cannot foresee any conflicts or issues emerging.

Sh4rK · 2020-10-13T22:24:46Z

Hey everyone,

This seems like a relatively small change, but would help greatly in writing wasm modules for Deno, is there anything blocking this? Is there anything to do to get it merged?

Sh4rK · 2020-10-13T22:46:06Z

src/command/utils.rs

+        Target::Deno => {}
+        _ => gitignore_contents.push_str("*")
+    }
+    fs::write(out_dir.join(".gitignore"), gitignore_contents)?;


Instead of writing an empty file, it would be cleaner to not write it at all, I think.

But also, I'm nut sure this is necessary, why would targeting Deno be different in this regard? You might want to package up the resulting files into a separate repository (equivalent to putting it on npm for node), but at the same time want to keep the rust lib's source clean.

Having a wildcard .gitignore doesn't make sense in any other way, you obviously want to keep the results somewhere.

I think this was the default behavior, they just removed that option for deno
the deleted line in 41 wrote * to .gitignore in every case, but @juzi5201314 probably concluded that it is not relevant for deno
this is still the state in the main branch (as of 2nd Feb, 2022)
and in the latest release atm
I agree about preferring not to create the .gitignore at all, but keeping the * for all other targets is just keeping the same behavior
changing this behavior is outside of the scope of this pr :)

I agree that we should leave create_pkg_dir as it is.
I could see one improvement to the status quo where we could read the current contents of an existing .gitignore before deleting the directory and then writing them back afterwards so modifications made by the user persist, but this should be a separate PR if it is a desired feature.

My comment is not very clear, reading it again now. I think my point was that this function should be left as-is, no need to special case it for Deno.

olanod · 2020-10-22T14:18:17Z

This looks helpful, is it missing anything?

Mipsters · 2020-11-21T23:07:39Z

I am using this branch, couple of problems I had

wasm-pack build --help didn't list deno as a target option
the .js generated in pkg doesn't manipulate the file path correctly for me
(may be related to the fact that I use git bash as my main console)
1. I use windows, yet the path string still comes with / and not \, so Deno.build.os === 'windows' ? '\\' : '/' give a wrong result
2. new URL(import.meta.url).pathname gives a path with \ before C: for some reason?

except for this, it works perfectly!
took me some time to realize that I needed the .js suffix in the import in my deno code, but it did work!

edit: after thinking about it a bit, the problems in 2 are probably problems in wasm-bingen and not this pr
sorry if that's the case

mindon · 2021-09-02T01:44:21Z

After another year ... it's not supported yet, the deno target

egfx-notifications · 2022-02-08T16:42:10Z

I'd very much like to see this merged. Is anything holding us back? I'd be happy to provide the necessary adjustments if required.

egfx-notifications · 2022-02-08T16:47:01Z

I am using this branch, couple of problems I had
1. `wasm-pack build --help` didn't list `deno` as a target option

2. the `.js` generated in pkg doesn't manipulate the file path correctly for me
   (may be related to the fact that I use git bash as my main console)
   
   1. I use windows, yet the path string still comes with `/` and not `\`, so `Deno.build.os === 'windows' ? '\\' : '/'` give a wrong result
   2. `new URL(import.meta.url).pathname` gives a path with `\` before `C:` for some reason?
except for this, it works perfectly! took me some time to realize that I needed the .js suffix in the import in my deno code, but it did work!

edit: after thinking about it a bit, the problems in 2 are probably problems in wasm-bingen and not this pr sorry if that's the case

@Mipsters Your point 2.2 seems to be fixed in the current wasm-bindgen release, the path is concatenated differently now.
2.1 I don't get right now, is Deno.build.os === 'windows' ? '\\' : '/' something you use in your code or something that wasm-bindgen created? Either way, I can't see this in output of current wasm-bindgen.
Your first point would definitely be an improvement to this PR.

Mipsters · 2022-02-09T07:49:51Z

@egfx-notifications wow, I barely remember what I did then, but I think that the ternary condition was not mine
if I remember correctly, it was of some inner library, don't remember if it was part of the pr or something else
you can run a simple script to check if this is resolved, if Deno.build.os === 'windows' is true on windows then there is no issue
if you don't have a windows machine available, hit me up, but it'll take time for me to remind myself how to use any of this things...

egfx-notifications · 2022-02-09T08:37:36Z

@egfx-notifications wow, I barely remember what I did then, but I think that the ternary condition was not mine if I remember correctly, it was of some inner library, don't remember if it was part of the pr or something else you can run a simple script to check if this is resolved, if Deno.build.os === 'windows' is true on windows then there is no issue if you don't have a windows machine available, hit me up, but it'll take time for me to remind myself how to use any of this things...

Then this seems to be fixed now. Tried cmd, Powershell and Git Bash on Windows, all give the correct result.

egfx-notifications

As stated in the comments, please remove the modifications to create_pkg_dir.

Please also add deno to the list here so it will be listed as available option in the --help output.

egfx-notifications · 2022-02-09T08:52:40Z

src/command/utils.rs

+        Target::Deno => {}
+        _ => gitignore_contents.push_str("*")
+    }
+    fs::write(out_dir.join(".gitignore"), gitignore_contents)?;


I agree that we should leave create_pkg_dir as it is.
I could see one improvement to the status quo where we could read the current contents of an existing .gitignore before deleting the directory and then writing them back afterwards so modifications made by the user persist, but this should be a separate PR if it is a desired feature.

drager · 2023-07-05T05:09:36Z

Fixed in #1117.

juzi5201314 added 4 commits August 27, 2020 12:09

Support deno target

4f1cc33

rustfmt

b112eae

Delete * in .gitignore of Deno target drager#909

c0cc625

Fix the error of create_pkg_dir in tests

40660ac

OliverBrotchie approved these changes Aug 30, 2020

View reviewed changes

Sh4rK reviewed Oct 13, 2020

View reviewed changes

Mipsters mentioned this pull request Nov 21, 2020

wasm-pack prints wat syntax for a very long time #938

Closed

oplik0 mentioned this pull request Dec 6, 2020

what needs to happen for the next release of wasm-pack #929

Closed

Merge branch 'rustwasm:master' into master

1c59725

egfx-notifications suggested changes Feb 9, 2022

View reviewed changes

egfx-notifications mentioned this pull request Mar 2, 2022

Make Deno target available #1117

Merged

3 tasks

drager closed this Jul 5, 2023

Conversation

juzi5201314 commented Aug 27, 2020

Uh oh!

OliverBrotchie commented Aug 30, 2020

Uh oh!

juzi5201314 commented Aug 30, 2020

Uh oh!

OliverBrotchie left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sh4rK commented Oct 13, 2020

Uh oh!

Sh4rK Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

Mipsters Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

egfx-notifications Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

Sh4rK Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

olanod commented Oct 22, 2020

Uh oh!

Mipsters commented Nov 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mindon commented Sep 2, 2021

Uh oh!

egfx-notifications commented Feb 8, 2022

Uh oh!

egfx-notifications commented Feb 8, 2022

Uh oh!

Mipsters commented Feb 9, 2022

Uh oh!

egfx-notifications commented Feb 9, 2022

Uh oh!

egfx-notifications left a comment

Choose a reason for hiding this comment

Uh oh!

egfx-notifications Feb 9, 2022

Choose a reason for hiding this comment

Uh oh!

drager commented Jul 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

OliverBrotchie left a comment •

edited

Loading

Mipsters Feb 9, 2022 •

edited

Loading

Mipsters commented Nov 21, 2020 •

edited

Loading