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

Universal reader #499

Merged
merged 9 commits into from
Jan 9, 2020
Merged

Universal reader #499

merged 9 commits into from
Jan 9, 2020

Conversation

antfu
Copy link
Member

@antfu antfu commented Jan 7, 2020

Goal

To make importing script much easier. #154 #100

Solutions

  • A new option for compile - importPaths: string[], providing a list of directories paths for searching modules to import.
  • Both local files and online files are allowed. (for example, you can specify the importPaths to ["https://raw.githubusercontent.com/LingDong-/wenyan-lang/master/examples/"] to load examples from github.
  • The new reader will search in the order of importPath, the first hit will get returned.
  • The cli will automatically add the script parent dir and current working dir to importPaths
  • The runtime will automatically add the root for the website to importPaths

Examples

The following example will search for local examples first, if not found, will fallback to online examples.

const compiled = await compile(src, {
  importPath: [
    "../examples",
    "https://raw.githubusercontent.com/LingDong-/wenyan-lang/master/examples",
  ]
})

Changes

  • reader, extractMacros become async functions in order to wait for requests.
  • compileAsync, executeAsync is added.

Considerations

  • Should we add --allow-http options (default to false) for better secure purpose (loading arbitrate code from web can be vulnerable )
  • Maybe we can also have something like trustedHosts and including https://raw.githubusercontent.com/LingDong-/wenyan-lang/master to load from official repo etc.

TODO

  • Async
  • Reader
  • use axios instead of fetch() for better compiltbily.
  • Async version compileAsync executeAsync (https://github.com/Yukaii/synchronized-promise)
  • AllowHttp
  • TustedHosts
  • Request timeout
  • Tests
  • CLI changes
  • Runtime changes
  • IDE/Site tests

Please feel free to leave any comments!

@LingDong-
Copy link
Member

LingDong- commented Jan 7, 2020

👍Sounds like a good idea!

Shall we make a sync version as well as the async version, like in nodejs's fs.readFileSync etc.? I think especially for non-browser usage, sync behavior is sometimes more desirable. Sync requests in the browser is also doable, though gives a warning about deprecation:

var req = new XMLHttpRequest().
req.open( "GET", url, false ); // false for synchronous request

The --allow-http and trustedHosts options sound good too.

Are we going to stop exposing the reader option? We could do so since importPaths seems to cover all cases, but we might also keep it for more advanced users.

@antfu
Copy link
Member Author

antfu commented Jan 7, 2020

Shall we make a sync version as well as the async version, like in odejs's fs.readFileSync etc.?

Agreed! I think we can use async in the external and expose a new blocking method. Then we don't need to worry to much about supporting both :)

Are we going to stop exposing the reader option? We could do so since importPath seems to cover all cases, but we might also keep it for more advanced users.

Sure. But maybe we need further discussion about this. The current IDE uses an bundled examples object for the reader. Should we change it to http requests or we may expose a new option for injecting the import context once we stop exposing the reader option.

@antfu
Copy link
Member Author

antfu commented Jan 7, 2020

Should we use

  • compile and compileSync (BREAKING CHANGE)

or

  • compile and compileAsync

IMO, I would prefer the former one since it better matches with node's APIs. But on the other hand, the latter seems more straightforward.

@LingDong-
Copy link
Member

LingDong- commented Jan 7, 2020

Personally I like compile and compileAsync better. Maybe we can also additionally alias compileSync to compile, but not sure if it will be a source of convenience or a confusion :P

@antfu
Copy link
Member Author

antfu commented Jan 7, 2020

So maybe let's go with compile and compileAsync then it will not break the current code 😄

@antfu
Copy link
Member Author

antfu commented Jan 7, 2020

Bad News: making async function sync seems to be impossible in the browser.

I was trying to make compile from compileAsync using https://www.npmjs.com/package/synchronized-promise and it works fine in node. It actually using some C++ binary to do the hack and there is no luck for the browser. (I did some research and there seems to be not alternative for that)

Maybe we should use compile and compileSync and make compileSync an option for the node side?

@LingDong-
Copy link
Member

Hi @antfu, I think you could do this:

function fetchTextSync(theUrl){
    var xmlHttp = new XMLHttpRequest();
    xmlHttp.open( "GET", theUrl, false ); // false for synchronous request
    xmlHttp.send( null );
    return xmlHttp.responseText;
}

console.log(fetchTextSync("https://raw.githubusercontent.com/LingDong-/wenyan-lang/master/lib/%E7%AE%97%E7%B6%93.wy"))
// prints the content of 算經 to the console.

I guess in compile you can check if it's in browser, and call this method above if so. Let me know if it works, thanks!

@antfu
Copy link
Member Author

antfu commented Jan 8, 2020

Normally I would not suggest to use deprecating apis, but your solution seems to work perfectly fine. I have changed all the functions back to sync mode as they did before.👍

@antfu antfu marked this pull request as ready for review January 8, 2020 02:20
@antfu
Copy link
Member Author

antfu commented Jan 8, 2020

It's ready! :P

@antfu antfu changed the title RFC: universal reader Universal reader Jan 8, 2020
@LingDong- LingDong- merged commit 02b31e0 into wenyan-lang:master Jan 9, 2020
@antfu antfu mentioned this pull request Jan 9, 2020
LingDong- added a commit that referenced this pull request Jan 20, 2020
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.

None yet

2 participants