meta: Don't exclude 'loophole' or 'pegjs' packages #206
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We need these when typing a "tab" character in a document or file. (Without 'loophole' and 'pegjs' in the app bundle, 'snippets' tries and fails to require() those packages, and it errors out every time you type a tab character.) Explanation: Our PegJS files _aren't_ pre-compiled at the moment. We removed the old build scripts that used to transpile these files during the build, and without replacing that functionality yet. So... When a certain PegJS --> JS file isn't available in the 'snippets' package (because it hasn't been transpiled to plain JS yet yet), 'snippets' require()s some modules that let it live-transpile the needed code, or at least that's what I think it's doing in that scenario. If the modules it conditionally require()s in that scenario aren't in the Pulsar bundle, due to being explicitly excluded, then we get a require() error, since they legitimately aren't in the bundle.
|
I can also confirm that this issue occurs in the newest version of master. I will check if this PR fixes the issue (Intel Mac). |
|
Update: this PR fixes the issue for me. I don't see any other side effects from the changes. |
confused-Techie
approved these changes
Dec 10, 2022
There was a problem hiding this comment.
Appreciate you catching this oversight. Now ideally we could build our transpiled pegjs files during a GitHub action so that there is always a normal js file available.
But until such time as that happens lets merge this one to get things working.
Thanks as always for the contribution!
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requirements for Contributing a Bug Fix (click to expand):
Identify the Bug
When trying to type a tab character in a document or file, after #201 was merged, there is an error:
Screenshot (click to expand):
Screenshot (click to expand):
Explanation as for why this happens:
There is a file
snippet-body.pegjswhich 'snippets' expects to have been compiled from .pegjs to .js. If it can't require the plain .js file successfully (it assumes this is because the .pegjs file hasn't been transpiled to plain JS yet, which is correct in our case)... then it appears to fall back on live-transpiling the .pegjs file using the 'pegjs' module.See: https://github.com/pulsar-edit/snippets/blob/388d9b63cacbc124652e9cec6ce4be6e3d6d46bf/lib/snippet-body-parser.js#L2-L12
So we need to have the 'pegjs' module (and 'loophole', whatever that is) in order to load 'snippets' package properly and restore the ability to type tab characters. (The app appears to trigger the 'snippets' package to activate every time you type a Tab? Which I understand is for code autocompletion purposes.)
Description of the Change
Don't exclude 'loophole' or 'pegjs' packages from the app bundle. Allows the 'snippets' package to work again.
Alternate Designs
We can attempt to compile the PegJS files to plain JS, like the old build scripts used to do. See: script/lib/transpile-peg-js-paths.js at Atom core repo.
At which point we apparently wouldn't need these packages.
Possible Drawbacks
Increases the final app bundle size by some 0.2 Megabytes, after compression, adds more files to the final bundle.
Verification Process
Tested CI binaries with/without these changes on my Windows 10 machine:
masterwith #201: Affected by the tab bug described at the top of this PR. ❌Release Notes
Fix a bug when typing a tab character in a document or file