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

Add postinstall hook to remove the webworker-threads module #244

Merged
merged 2 commits into from Apr 11, 2018

Conversation

Projects
None yet
2 participants
@jasonrudolph
Member

jasonrudolph commented Apr 11, 2018

In an attempt to resolve the problem described in #67 (comment), this pull request adds a postinstall hook to delete the webworker-threads module as described in #67 (comment).

This is admittedly a hack, and I'm not certain that it will resolve the issue, but we'll give it a shot.

If this does resolve the issue, we can consider it a temporary workaround, and we can remove this workaround when the spelling-manager module no longer depends on the natural module (#67 (comment)), or when the natural module makes the webworker-threads module a peer dependency (NaturalNode/natural#368) instead of an optional dependency.

/fyi @dmoonfire @maxbrunsfeld

jasonrudolph added some commits Apr 11, 2018

@jasonrudolph

This comment has been minimized.

Member

jasonrudolph commented Apr 11, 2018

I've tested this on macOS, Windows 10, and Ubuntu as a sanity check regarding cross-platform compatibility. In each case, I did the following:

git clone https://github.com/atom/spell-check
cd spell-check
apm link
apm install

Then, verify that node_modules/webworker-threads is present.

Then:

git checkout jr-investigate-67
apm install

Then, verify that node_modules/webworker-threads is not present.

Then, open Atom and run the "Spell Check: Toggle" command on a file to verify that spell check is still working.

@jasonrudolph jasonrudolph merged commit 87a12ca into master Apr 11, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasonrudolph jasonrudolph deleted the jr-investigate-67 branch Apr 11, 2018

@jasonrudolph

This comment has been minimized.

Member

jasonrudolph commented Apr 11, 2018

It doesn't seem like this change is going to have the desired effect. 😩 While the approach in this pull request successfully removed the webworker-threads module in the scenario described in #244 (comment), it doesn't seem to remove the module when bundling the spell-check package as part of a full Atom build.

I published a patch version of this package (0.73.2) and updated atom/atom to use that version (atom/atom@282fd85). I then downloaded resulting atom-mac.zip built by CI for that commit. Unfortunately, webworker-threads is still present:

$ tree /Applications/Atom.app/Contents/Resources/app.asar.unpacked/node_modules/webworker-threads
/Applications/Atom.app/Contents/Resources/app.asar.unpacked/node_modules/webworker-threads
└── build
    └── Release
        └── WebWorkerThreads.node

2 directories, 1 file

I'll see if I can find an approach that will meet our needs. If anyone has ideas, definitely let me know. 😅🙏

@dmoonfire

This comment has been minimized.

Contributor

dmoonfire commented Apr 11, 2018

I added an issue to spelling-manager to get rid of the dependency: dmoonfire/spelling-manager-js#4.

jasonrudolph added a commit that referenced this pull request Apr 12, 2018

Revert #244
This reverts commits bceff78 and
fc81e5d.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment