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

Align with current latest alphagov/accessible-autocomplete repo #28

Merged
merged 5 commits into from
Apr 12, 2024

Conversation

andygout
Copy link
Contributor

@andygout andygout commented Mar 29, 2024

When running npm i or npm ci I encounter the following error:

npm ERR! code 1
npm ERR! code 1
npm ERR! path /{path}/accessible-autocomplete/node_modules/fibers
npm ERR! command failed
npm ERR! command sh -c node-gyp rebuild
npm ERR! CXX(target) Release/obj.target/fibers/src/fibers.o
npm ERR! gyp info it worked if it ends with ok
npm ERR! gyp info using node-gyp@9.4.0
npm ERR! gyp info using node@20.8.1 | darwin | x64
npm ERR! gyp info find Python using Python version 3.9.7 found at "/{path}/opt/anaconda3/bin/python3"
npm ERR! gyp info spawn /{path}/opt/anaconda3/bin/python3
npm ERR! gyp info spawn args [
npm ERR! gyp info spawn args   '/{path}/.volta/tools/image/node/20.8.1/lib/node_modules/npm/node_modules/node-gyp/gyp/gyp_main.py',
npm ERR! gyp info spawn args   'binding.gyp',
npm ERR! gyp info spawn args   '-f',
npm ERR! gyp info spawn args   'make',
npm ERR! gyp info spawn args   '-I',
npm ERR! gyp info spawn args   '/{path}/accessible-autocomplete/node_modules/fibers/build/config.gypi',
npm ERR! gyp info spawn args   '-I',
npm ERR! gyp info spawn args   '/{path}/.volta/tools/image/node/20.8.1/lib/node_modules/npm/node_modules/node-gyp/addon.gypi',
npm ERR! gyp info spawn args   '-I',
npm ERR! gyp info spawn args   '/{path}/Library/Caches/node-gyp/20.8.1/include/node/common.gypi',
npm ERR! gyp info spawn args   '-Dlibrary=shared_library',
npm ERR! gyp info spawn args   '-Dvisibility=default',
npm ERR! gyp info spawn args   '-Dnode_root_dir=/{path}/Library/Caches/node-gyp/20.8.1',
npm ERR! gyp info spawn args   '-Dnode_gyp_dir=/{path}/.volta/tools/image/node/20.8.1/lib/node_modules/npm/node_modules/node-gyp',
npm ERR! gyp info spawn args   '-Dnode_lib_file=/{path}/Library/Caches/node-gyp/20.8.1/<(target_arch)/node.lib',
npm ERR! gyp info spawn args   '-Dmodule_root_dir=/{path}/accessible-autocomplete/node_modules/fibers',
npm ERR! gyp info spawn args   '-Dnode_engine=v8',
npm ERR! gyp info spawn args   '--depth=.',
npm ERR! gyp info spawn args   '--no-parallel',
npm ERR! gyp info spawn args   '--generator-output',
npm ERR! gyp info spawn args   'build',
npm ERR! gyp info spawn args   '-Goutput_dir=.'
npm ERR! gyp info spawn args ]
npm ERR! gyp info spawn make
npm ERR! gyp info spawn args [ 'BUILDTYPE=Release', '-C', 'build' ]
npm ERR! ../src/fibers.cc:27:65: error: no member named 'kFinalizer' in 'v8::WeakCallbackType'
npm ERR!                 handle.SetWeak(val, WeakCallbackShim<F, P>, WeakCallbackType::kFinalizer);
npm ERR!                                                             ~~~~~~~~~~~~~~~~~~^
npm ERR! 1 error generated.
npm ERR! make: *** [Release/obj.target/fibers/src/fibers.o] Error 1
npm ERR! gyp ERR! build error
npm ERR! gyp ERR! stack Error: `make` failed with exit code: 2
npm ERR! gyp ERR! stack     at ChildProcess.onExit (/{path}/.volta/tools/image/node/20.8.1/lib/node_modules/npm/node_modules/node-gyp/lib/build.js:203:23)
npm ERR! gyp ERR! stack     at ChildProcess.emit (node:events:514:28)
npm ERR! gyp ERR! stack     at ChildProcess._handle.onexit (node:internal/child_process:294:12)
npm ERR! gyp ERR! System Darwin 23.4.0
npm ERR! gyp ERR! command "/{path}/.volta/tools/image/node/20.8.1/bin/node" "/{path}/.volta/tools/image/node/20.8.1/lib/node_modules/npm/node_modules/node-gyp/bin/node-gyp.js" "rebuild"
npm ERR! gyp ERR! cwd /{path}/accessible-autocomplete/node_modules/fibers
npm ERR! gyp ERR! node -v v20.8.1
npm ERR! gyp ERR! node-gyp -v v9.4.0
npm ERR! gyp ERR! not ok

npm ERR! A complete log of this run can be found in: /{path}/.npm/_logs/2024-03-29T16_30_46_994Z-debug-0.log

The error seems to be originating from the @wdio/sync dev dependency — a package no longer used in https://github.com/alphagov/accessible-autocomplete, from which this repo was forked a few years ago.

This PR does the following:

This means that we retain an installable/usable fork of accessible-autocomplete to which we can continue to add modifications (such as this: #29).


Long-term solution

Having a forked repo is not ideal, and the reason for doing so is documented in

https://github.com/Financial-Times/origami/blob/b632ca6643e93a720ed766bdce4e8eb0747a1b7b/components/o-autocomplete/src/js/autocomplete.js#L1-L8

// We use our own fork of accessible-autocomplete because the main package is not being actively maintained and has bugs which we needed to fix
// There is a changelog for the fixes we've added -- https://github.com/Financial-Times/accessible-autocomplete/blob/master/CHANGELOG.md#210---2021-05-24
// Below are the pull-requests to accessible-autocomplete which would fix the bugs:
// alphagov/accessible-autocomplete#497
// alphagov/accessible-autocomplete#491
// alphagov/accessible-autocomplete#496
// If the above pull-requests are merged and published, then we can stop using our fork
import accessibleAutocomplete from '@financial-times/accessible-autocomplete';

I inspected those PRs to see if they had been merged (or that the bugs they sought to fix had been fixed elsewhere):

alphagov/accessible-autocomplete#497

alphagov/accessible-autocomplete#491

  • This bug is still present
  • [May 24, 2021] Really appreciate your work, however at the moment we don't have capacity to deal with issues and pull requests on this repo unfortunately: Types of support we can provide. alphagov/accessible-autocomplete#430

  • Activity on this repo seems to have picked up a fair amount since the start of this year so might be worth making another request for this PR to be looked at? If it is merged then we can delete this fork and depend on the source repo.

alphagov/accessible-autocomplete#496

Financial-Times/accessible-autocomplete #6

In addition, this fork now also contains these changes: #6. The external contributor who added them also tried to add them to the source repo here alphagov/accessible-autocomplete#500 but that PR was closed because:

A more generic solution has been implemented in alphagov/accessible-autocomplete#591 for configure that attribute, so I'll be closing this PR.

To put us closer to being able to close this fork in favour of using the source repo, I've removed the changes in #6 in favour of those in alphagov/accessible-autocomplete#591.

The release for these changes can be a major version, making clear that this feature has been removed and that any usage of the ariaLabelledBy option will need to switch to applying an aria-labelledby property to the ul element via the menuAttributes option.

Release

Given this PR includes breaking changes, I propose releasing this as a major version.

Copy link
Contributor

@notlee notlee left a comment

Choose a reason for hiding this comment

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

🙇 🙇 🙇

@notlee notlee merged commit 1b17aac into master Apr 12, 2024
2 checks passed
@notlee notlee deleted the align-with-current-latest-alphagov-repo branch April 12, 2024 08:55
notlee added a commit that referenced this pull request Apr 12, 2024
See #28
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.

2 participants