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

support require json file #580

Merged
merged 1 commit into from
Sep 10, 2024
Merged

support require json file #580

merged 1 commit into from
Sep 10, 2024

Conversation

ahaoboy
Copy link
Contributor

@ahaoboy ahaoboy commented Sep 8, 2024

Description of changes

Support require json files

console.log( require('package.json') )

Checklist

  • Created unit tests in tests/unit and/or in Rust for my feature if needed
  • Ran make fix to format JS and apply Clippy auto fixes
  • Made sure my code didn't add any additional warnings: make check
  • Added relevant type info in types/ directory
  • Updated documentation if needed (API.md/README.md/Other)

Copy link
Contributor

@richarddavison richarddavison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great addition but should be done at import level instead of only supporting require.

@ahaoboy
Copy link
Contributor Author

ahaoboy commented Sep 9, 2024

This is a great addition but should be done at import level instead of only supporting require.

Do you mean support for import a from './a.json'?
This should implement a JSONLoader, but nodejs not support this syntax

@richarddavison
Copy link
Contributor

This is a great addition but should be done at import level instead of only supporting require.

Do you mean support for import a from './a.json'? This should implement a JSONLoader, but nodejs not support this syntax

It does not but does through import assertions. I think we can allow it in LLRT as well.
Also left another comment in the PR regarding evaluation of JSON.

llrt_core/src/vm.rs Outdated Show resolved Hide resolved
@ahaoboy
Copy link
Contributor Author

ahaoboy commented Sep 9, 2024

It does not but does through import assertions. I think we can allow it in LLRT as well.

This may be more troublesome than expected due to the confusing behavior of cjs and esm

If you use the json file as a js file that exports a js object by default

{ "a": 1 }
export default { "a": 1 } 

Then what we get is an object like this

const a = require('./a.json')

// { default: { a: 1} }

@richarddavison
Copy link
Contributor

If you use the json file as a js file that exports a js object by default

What do you mean by this? :)

@ahaoboy
Copy link
Contributor Author

ahaoboy commented Sep 9, 2024

What do you mean by this? :)

It might be easier to implement json import for require and import separately

use rquickjs::{loader::Loader, Ctx, Error, Module, Result};

#[derive(Debug)]
pub struct JSONLoader;

impl Loader for JSONLoader {
    fn load<'js>(&mut self, ctx: &Ctx<'js>, path: &str) -> Result<Module<'js>> {
        if !path.ends_with(".json") {
            return Err(Error::new_loading(path));
        }
        let source = std::fs::read_to_string(path)?;
        Module::declare(ctx.clone(), path, format!("export default ({source})"))
    }
}

@richarddavison
Copy link
Contributor

Right I see what you mean. It might be better to have two separate implementations since there is no way to return an already created object from a module.

I would for the require function use json_parse and for the json loader use:

Module::declare(ctx.clone(), path, ["export default ", &source].concat())

@richarddavison richarddavison merged commit 9a4afce into awslabs:main Sep 10, 2024
9 checks passed
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.

2 participants