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

Resolver refactor #202

Merged
merged 7 commits into from
May 15, 2023
Merged

Resolver refactor #202

merged 7 commits into from
May 15, 2023

Conversation

ColinEberhardt
Copy link
Collaborator

@ColinEberhardt ColinEberhardt commented May 5, 2023

Given that we have a few issues (#198 #195 #171) relating to the resolver, I wanted to do a bit of a refactor before tackling the specific bugs. This PR doesn't address them directly, but should make it easier to fix them in future. Changes include:

  • moving shell logic into its own module
  • resolver now returns an object, with a dispose function for cleanup
  • unit tested!
  • npm install now uses temp folder rather than mutating the current project package.json

TODO

  • fix the build failures!
  • move isUrl into a util module

@ms14981
Copy link
Contributor

ms14981 commented May 10, 2023

Done a git bisect to work out why the smoke test is failing. It appears that this commit was where it started breaking: eb05bd6

I am testing with ./src/index.js forge https://petstore3.swagger.io/api/v3/openapi.json ../openapi-forge-typescript/.

  if (fs.existsSync(generatorPathOrUrl)) {
    validateGenerator(generatorPathOrUrl);
    return {
      // path: generatorPathOrUrl // Returns as relative path - does not seem to work
      path: path.resolve(generatorPathOrUrl), // Returns as absolute path - seems to work
      dispose: () => {},
    };
  }

It seems like the code using getGenerator is expecting an absolute path, because forge seems to work again when we switch back to using path.resolve.

It looks like we're mocking out getGenerator in the generate unit tests. It might be worth either changing this test to mock at a lower level or add a test in the generatorResolver.

@ColinEberhardt
Copy link
Collaborator Author

It seems like the code using getGenerator is expecting an absolute path, because forge seems to work again when we switch back to using path.resolve.

Thanks for spotting - I've ensured that the generator resolver always returns an absolute path, which fixes the tests.

@github-actions
Copy link

🎉 This PR is included in version 1.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ColinEberhardt ColinEberhardt deleted the resolver-updates branch October 26, 2023 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants