-
Notifications
You must be signed in to change notification settings - Fork 18
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
add initial support for wasm target #92
Open
dgmachado
wants to merge
7
commits into
fmeringdal:main
Choose a base branch
from
dgmachado:add-wasm-target
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
dbb25c0
add initial support for wasm target
dgmachado f6b499c
add nodejs wasm example
dgmachado 4738b66
wasm integration refactoring
dgmachado 446d53a
add clippy lint into wasm integration
dgmachado 883c735
feat(wasm); created wasm module inside the rrule crate
dgmachado 1a6e951
code review fixes
dgmachado 391b3f2
code review fixes
dgmachado File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
install-wasm-pack: | ||
curl https://rustwasm.github.io/wasm-pack/installer/init.sh -sSf | sh | ||
|
||
build-wasm-nodejs: | ||
wasm-pack build --target nodejs --features "wasm" | ||
|
||
test-wasm-on-nodejs: | ||
node examples/wasm/nodejs/app.js | ||
|
||
build-wasm-web: | ||
wasm-pack build --target web --features "wasm" | ||
|
||
test-wasm-on-web-browser: | ||
npx http-server -o /examples/wasm/web/index.html |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
const { get_all_date_recurrences } = require('../../../pkg/rrule.js'); | ||
|
||
const http = require('http'); | ||
const url = require('url'); | ||
const hostname = '127.0.0.1'; | ||
const port = 3000; | ||
|
||
const server = http.createServer((req, res) => { | ||
const queryObject = url.parse(req.url,true).query; | ||
res.statusCode = 200; | ||
res.setHeader('Content-Type', 'text/plain'); | ||
|
||
const rule_set = "DTSTART:20120201T093000Z\nRRULE:FREQ=DAILY;COUNT=3"; | ||
const data = get_all_date_recurrences(rule_set, 100); | ||
console.log(data); | ||
res.end(data.toString()); | ||
}); | ||
|
||
server.listen(port, hostname, () => { | ||
console.log(`Server running at http://${hostname}:${port}/`); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
<!DOCTYPE html> | ||
<html> | ||
<head> | ||
<meta content="text/html;charset=utf-8" http-equiv="Content-Type"/> | ||
<meta http-equiv="cache-control" content="max-age=0" /> | ||
<meta http-equiv="cache-control" content="no-cache" /> | ||
</head> | ||
<body> | ||
|
||
<script type="module"> | ||
import init, { get_all_date_recurrences } from '../../../pkg/rrule.js'; | ||
|
||
async function run_wasm() { | ||
await init(); | ||
const ruleSet = "DTSTART:20120201T093000Z\nRRULE:FREQ=DAILY;COUNT=3"; | ||
const data = get_all_date_recurrences(ruleSet, 100); | ||
console.log(data); | ||
alert(data); | ||
} | ||
run_wasm(); | ||
</script> | ||
</body> | ||
</html> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
use wasm_bindgen::prelude::*; | ||
|
||
use crate::{RRuleSet, RRuleError}; | ||
|
||
/// Get all recurrences of the rrule | ||
/// | ||
/// # Arguments | ||
/// | ||
/// * `rule_set` - List of rrules | ||
/// | ||
/// * `limit` - Limit must be set in order to prevent infinite loops | ||
#[wasm_bindgen] | ||
pub fn get_all_date_recurrences(rule_set: &str, limit: Option<u16>) -> Result<Vec<JsValue>, JsError> { | ||
let rrule_result: Result<RRuleSet, RRuleError> = rule_set.parse(); | ||
match rrule_result { | ||
Ok(rrule) => { | ||
/// Set hard limit in case of infinitely recurring rules | ||
let rule_set_collection = rrule.all(limit.unwrap_or(100)); | ||
let result: Vec<JsValue> = rule_set_collection.dates | ||
.into_iter() | ||
.map(|s| { | ||
JsValue::from_str(&s.to_string()) | ||
}) | ||
.collect(); | ||
Ok(result) | ||
}, | ||
Err(e) => Err(JsError::from(e)) | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this also contain the default "lib" crate type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. In my tests, it wasn't necessary. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should contain the default "lib" as well. I will need to read up on this setting more before merging this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fmeringdal setting the Cargo.toml file as:
Once I run cargo build it's showing the below error:
How do you recommend to fix this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it build with just "lib"? Why was "lib" replaced with ["cdylib", "rlib"]. I am not that familiar with how WASM internals work and don't want to merge anything I don't fully understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fmeringdal @antoinepairet
Assuming in our context the below command as Rust Build:
And the below command as Wasm Build:
wasm-pack build --release --target web --features "wasm"
We can analyze the different outputs:
1.a) the Rust Build works.
1.b) the Wasm Build fails:
2.a) the Rust Build and Wasm Build command fail and return the same output:
3.a) the Rust Build and Wasm Build command build successfully (the current PR is using this config).
I've found two docs that does not answer this type of problem:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the context @dgmachado . I think the wasm build is fine, my main concern is the implications of the Rust build and how it might affect applications relying on this library. Before resolving this discussion I will have to do a deeper dive into WASM myself to understand what is going on and why this is required. Sorry for holding up this PR, but I can't merge anything that I don't fully understand as I have to maintain this.