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

fix(rulesets): require node_module #1029

Merged
merged 4 commits into from
Mar 24, 2020
Merged

fix(rulesets): require node_module #1029

merged 4 commits into from
Mar 24, 2020

Conversation

P0lip
Copy link
Contributor

@P0lip P0lip commented Mar 23, 2020

Fixes #1026

Checklist

  • Tests added / updated
  • Docs added / updated

Does this PR introduce a breaking change?

  • Yes
  • No

@P0lip P0lip added the t/bug Something isn't working label Mar 23, 2020
@P0lip P0lip requested a review from nulltoken March 23, 2020 18:57
@P0lip P0lip self-assigned this Mar 23, 2020
@nulltoken
Copy link
Contributor

@P0lip This PR triggers a warning during the building of the Spectral binary:

> [email protected]
> Targets not specified. Assuming:
  node10-linux-x64
> Fetching base Node.js binaries to PKG_CACHE_PATH
  fetched-v10.17.0-linux-x64   [] 0%  fetched-v10.17.0-linux-x64   [] 100%
> Warning Malformed requirement for 'p'
  /home/circleci/project/dist/rulesets/evaluators.js

@nulltoken
Copy link
Contributor

@P0lip Regarding the warning that pkg spits out, shouldn't we find a way to fail the build when that happens?

@P0lip
Copy link
Contributor Author

P0lip commented Mar 24, 2020

@P0lip This PR triggers a warning during the building of the Spectral binary:
@P0lip Regarding the warning that pkg spits out, shouldn't we find a way to fail the build when that happens?

Yeah, same on my end. Shouldn't be harmful though. It's due to that require.resolve call, but everything works fine.

@nulltoken
Copy link
Contributor

Yeah, same on my end. Shouldn't be harmful though. It's due to that require.resolve call, but everything works fine.

So your thoughts are: "If the test harness works ok, then the binary should have been properly packaged"? Good enough for me.

@nulltoken
Copy link
Contributor

@P0lip LGTM

@P0lip
Copy link
Contributor Author

P0lip commented Mar 24, 2020

So your thoughts are: "If the test harness works ok, then the binary should have been properly packaged"? Good enough for me.

I tested the binary manually as well.

@P0lip P0lip merged commit 9e2ca55 into develop Mar 24, 2020
@P0lip P0lip deleted the fix/require-module branch March 24, 2020 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot 'require' a module from node_modules/ in custom functions without providing a path
2 participants