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

Handle Web IDL #31

Merged
merged 6 commits into from
Sep 8, 2021
Merged

Handle Web IDL #31

merged 6 commits into from
Sep 8, 2021

Conversation

annevk
Copy link
Member

@annevk annevk commented Sep 2, 2021

See whatwg/webidl#1018 for context.

Disclaimer: I haven't actually gotten npm run pp-webidl -- --input index.html to run locally yet.

@annevk
Copy link
Member Author

annevk commented Sep 2, 2021

One thing this fails to do is run npm install. Is this the first time that is needed? Should I just run npm install each time or do we want to handle that differently? @domenic?

@domenic
Copy link
Member

domenic commented Sep 2, 2021

I think the best setup would be to not run npm install as part of deploy.sh or the makefile, but instead have the CI build do the installation using @actions/setup-node and then an npm install step.

So, that'd require some additional work in .github/workflows, I guess :-/.

@andreubotella
Copy link
Member

WebIDL's makefile was running webidl-pp for non-deploy builds, and its .pr-preview.json file had "post-processing": {"name": "webidl-grammar"}. The post-build steps will instead only run as part of make deploy. Now, the webidl-pp transformation isn't necessary on local or preview builds, since it will happen dynamically on page load anyway, but let's make sure that this isn't a difference that gets overlooked.

Also, there's the check-grammar.js script that makes sure the WebIDL grammar stays as LL(1). That should probably also run as part of the post-build steps.

@annevk annevk marked this pull request as ready for review September 3, 2021 14:47
@annevk
Copy link
Member Author

annevk commented Sep 3, 2021

I'm happy with this now.

I've not tackled .pr-preview.json. I did notice it, but I think at some point we probably want .pr-preview.json to use the output of the build process and not the output of Bikeshed/Wattsi. And as you note it is okay either way.

@annevk
Copy link
Member Author

annevk commented Sep 6, 2021

I'm not sure why I added the "do not merge yet" label, but I think this can be merged. I've also run it for whatwg/webidl#1018 and included the output therein.

I could change the node-version back to 16 if desired. I noticed we use 14 elsewhere which is why I went with that here. Up to @domenic.

Note that this PR now also adds the ability to run factory.py without generating PRs and with the ability to create changes for a single repository, which is useful for things that are being transitioned.

@annevk annevk mentioned this pull request Sep 6, 2021
19 tasks
@annevk annevk requested review from foolip and domenic September 7, 2021 11:00
@annevk annevk merged commit 25381fe into main Sep 8, 2021
@annevk annevk deleted the annevk/webidl branch September 8, 2021 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants