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

refactor: add pacote to resolve template name instead of arborist workaround #1115

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

akkshitgupta
Copy link

Description

  • add pacote dependency
  • cleanup arbortis mock and add new one for pacote
  • modify generator.js as suggested

Related issue(s) fixes: #1102

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

@derberg derberg changed the title chore: add pacote refactor: add pacote to resolve template name instead of arborist workaround Feb 13, 2024
Copy link

sonarcloud bot commented Feb 14, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@@ -0,0 +1,9 @@
const pacote = jest.genMockFromModule('pacote');
Copy link
Member

Choose a reason for hiding this comment

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

tests complain

ENOENT: no such file or directory, open 'node:fs/promises'

can you have a look?

Copy link
Author

Choose a reason for hiding this comment

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

It seems very weird to me. In my investigation, all the tests are running fine if I am not installing the pacote module and keeping all other things untouched.

When I run npm i pacote before running the tests, I get the same error. I need to figure out the possible reason.

Copy link
Author

Choose a reason for hiding this comment

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

The node version is the culprit here: #1115 (comment)

Also, several dependencies are deprecated, which results in failing tests. The project needs to be upgraded to a newer node version with updated dependencies. I suggest updating all the project dependencies to updated versions based on our requirements.

WDYT @derberg How should we move forward?

@lmgyuan
Copy link
Collaborator

lmgyuan commented Apr 19, 2024

@derberg @akkshitgupta Can you try deprecating the version of pacote to 15.1.1 in the package.js? I tried it in my copied branch of my forked repository, and it seems to pass the ubuntu test after I deprecated it.

Screenshot 2024-04-18 at 11 56 01 PM

Link to my PR
Screenshot 2024-04-18 at 11 56 38 PM

@derberg
Copy link
Member

derberg commented Apr 22, 2024

let's maybe wait for #1069 that will release new generator major release dropping support for older nodes where pacote may have issues?

@derberg
Copy link
Member

derberg commented Apr 24, 2024

@akkshitgupta please solve merge conflicts

@akkshitgupta
Copy link
Author

akkshitgupta commented Apr 25, 2024

Hello @derberg, after resolving conflicts, I am getting this error while running test while it was not the case before merging master. Can you have a look at here

image

Copy link
Member

derberg commented Apr 25, 2024

yeah, knows issue, here is a fix #1202

@derberg
Copy link
Member

derberg commented Apr 29, 2024

@akkshitgupta you can update your PR. There is a conflict with package-lock. Just solve it locally by running npm install with verbose flag, of course after first merging latest upstream to your branch, the installation will update package-lock

@derberg
Copy link
Member

derberg commented May 13, 2024

@akkshitgupta please have a look into failing tests

Copy link

sonarcloud bot commented May 13, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@akkshitgupta
Copy link
Author

Hey @derberg, its working now. It was an issue on the server side. on the previous update, there some network issue.test failed because of improper dependency installation.

@derberg
Copy link
Member

derberg commented May 13, 2024

@akkshitgupta still failing

@akkshitgupta
Copy link
Author

Apologies for the delay @derberg. I couldn't pay much attention due to some problems. I looked into it, but couldn't find the reason. i guess, it is failing because of the template version and I don't know how tk solve it.

@Gmin2
Copy link
Collaborator

Gmin2 commented Jul 1, 2024

Hey @akkshitgupta please resolve the conflicts, just change the file structure

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.

Improve arborist (npm installation) to have no hacks
4 participants