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

Upgrade to PEGjs 0.10… #15

Merged

Conversation

savetheclocktower
Copy link
Sponsor

…removing our dependency on loophole. (This was the weird eval thing. Newer versions of PEGjs no longer require usage of eval.

Also reintroduced the generated PEGjs file to improve startup time, giving us a second way to avoid eval. In theory we can move pegjs to devDependencies, but for now we can just try to load the compiled version and fall back to generating it manually from the .pegjs file if it isn't present.

…removing our dependency on `loophole`.

Also reintroduced the generated PEGjs file to improve startup time. In theory we
can move `pegjs` to `devDependencies`, but for now we can just try to load the
compiled version and fall back to generating it manually from the `.pegjs` file
if it isn't present.
Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

Overall this looks rad! Totally happy to see things get more up to date, and personally much prefer keeping our generated PegJS here, and move PegJS to our devDependencies, where if we get all of our package's to do this, we might be able to stop unpacking (or even installing) PegJS in our binaries, which can help save us some size on install.

Real fast though, what's the purpose of adding eslint as a devDependency? Was that already being used here and I've just missed it?

@savetheclocktower
Copy link
Sponsor Author

Real fast though, what's the purpose of adding eslint as a devDependency? Was that already being used here and I've just missed it?

It probably wasn’t, but likely only because this used to all be CoffeeScript. eslint is a devDependency on the main repo.

The coding styles vary between repos. Older code tends to enforce absence of semicolons, whereas new JS that hasn't been decaffeinated tends to mandate semicolons. At some point we'll want to harmonize all this, so bringing eslint into the repo is an intermediate step.

@savetheclocktower savetheclocktower merged commit 70bfe83 into pulsar-edit:master Jun 23, 2023
3 checks passed
@confused-Techie
Copy link
Member

Yeah I'm not against it at all. Just wondering if it was used much anywhere yet. But that sounds good to me. And totally agree we will need to find out what we prefer, likely setting up some sort of official styleguide, that we could all get behind and enforce via automated tools

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