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

Consider a better API for feeding in files #160

Open
domenic opened this issue Jan 25, 2020 · 3 comments
Open

Consider a better API for feeding in files #160

domenic opened this issue Jan 25, 2020 · 3 comments

Comments

@domenic
Copy link
Member

domenic commented Jan 25, 2020

#159 ran into an issue where the current API is not flexible enough to do targeted testing, because it operates on a directory level. This has always been a bit of a strange design; it's not very flexible and assumes a lot about the directory structure. I'd love to get opinions on what would be better designs.

Here are some ideas:

const transformer = new WebIDL2JS();

transformer.addSource("path/to/Attr.webidl", "path/to/Attr-impl.js");
// ... add the rest, one file at a time. The client is responsible for directory iteration.

await transformer.generate("outputDir");
const transformer = new WebIDL2JS({
  implPath(webidlFilePath) { return ...; }
  wrapperPath(webidlFilePath, implPath) { return ...; }
});

transformer.addFile("webidlFilesDir/Attr.webidl");

// Optionally add some of these as convenience methods:
transformer.addDirectory("webidlFilesDir");
transformer.addFromGlob("webidlFilesDir/**.webidl");

await transformer.generate();

Other ideas are welcome as well.

@ExE-Boss
Copy link
Contributor

In #208, I implemented addFile(…) and addDirectory(…) to match addSource(…), except with an optional impl path argument.


It also fixes the bug where:

const transformer = new WebIDL2JS();

transformer.addSource("path/to/Attr.webidl", "path/to/Attr-impl.js");
// ... add the rest, one file at a time. The client is responsible for directory iteration.

await transformer.generate("outputDir");

Would result in outputDir/Attr.js requiring path/to/Attr‑impl.js/Attr.js instead of path/to/Attr‑impl.js, because the wrapper generation code currently assumes that the impl path argument is always a directory.

@domenic
Copy link
Member Author

domenic commented Apr 22, 2020

I'd like us to consider and decide on the eventual shape of the whole API, and not just add things. In particular, I think addSource is a bad API with a bad name.

Can you describe how addFile and addDirectory work, including what arguments they take? And, if you disagree and think addSource should be kept, then can you include that as part of your description? Basically, before we consider working on a pull request, I'd like to see a concrete proposal, at least at the level of detail of API documentation.

@domenic
Copy link
Member Author

domenic commented Jun 28, 2021

Testing would be easier if we had a core API that didn't do file I/O at all, plus a wrapper API that did the file I/O. I guess that would be something like

const transformer = new WebIDL2JS();

transformer.add(idlString1, implModuleSpecifier1);
transformer.add(idlString2, implModuleSpecifier2);

for (const constructName of transformer.constructs()) {
  const result = transformer.generate(constructName);
}

const utils = transformer.utils();

Although this idea of using module specifiers for the impl is a bit fragile; it helps keep the pipeline pure string-to-string, but it makes it the caller's responsibility to get the exact path mapping right on disk. Maybe that's fine with a good wrapper API though...

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

No branches or pull requests

2 participants