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

Add support for absolute plugin paths #4533

Merged
merged 2 commits into from Jul 8, 2022
Merged

Conversation

ygoe
Copy link
Contributor

@ygoe ygoe commented Jun 17, 2022

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

Plugins can only be specified as package names or relative paths on the command line. This doesn't meet my requirements. When using rollup packaged in my tool application, I need to specify the absolute installation path of a plugin. No other solution will work for me.

I have verified that this change satisfies this requirement in my local installed copy of the code. I don't have any environment for TypeScript or rollup development, so I can only provide the backport of my patched JavaScript source code. I'm unable to provide anything more than this. It's a trivial change though and should be easy to review.

If necessary, I can try to locate the relevant documentation in this repository and update it, too.

@codecov
Copy link

codecov bot commented Jun 20, 2022

Codecov Report

Merging #4533 (43ea735) into master (60806b5) will decrease coverage by 0.02%.
The diff coverage is 33.33%.

❗ Current head 43ea735 differs from pull request most recent head 223ffc7. Consider uploading reports for the commit 223ffc7 to get more accurate results

@@            Coverage Diff             @@
##           master    #4533      +/-   ##
==========================================
- Coverage   98.89%   98.86%   -0.03%     
==========================================
  Files         208      208              
  Lines        7332     7334       +2     
  Branches     2094     2095       +1     
==========================================
  Hits         7251     7251              
- Misses         26       27       +1     
- Partials       55       56       +1     
Impacted Files Coverage Δ
cli/run/commandPlugins.ts 93.18% <33.33%> (-4.44%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 60806b5...223ffc7. Read the comment docs.

@lukastaegert lukastaegert force-pushed the patch-1 branch 6 times, most recently from 03fadcd to b08c3a6 Compare June 20, 2022 07:05
Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

While the approach looks reasonable, your implementation was referencing a file from the build output, which means subsequent builds would not have worked. Also, you did not add a test (which would have shown that your solution does not work).
I fixed this and extended an existing test to also work on Windows. Also, you should probably use pathToFileUrl instead of just prepending file:// as the latter was failing for me due to wrong path separators.
Please give the branch another go, if CI is green and it works for you I will release this. You should be able to install it and try it out via

npm install ygoe/rollup#patch-1

@ygoe
Copy link
Contributor Author

ygoe commented Jun 29, 2022

I cannot test this because the npm package fails to install. The lenghty error message says something about the command "husky" was not found. I don't know what that means.

@lukastaegert
Copy link
Member

I think as CI seems to be happy, I will merge this anyway.

@lukastaegert lukastaegert merged commit fbb86f5 into rollup:master Jul 8, 2022
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.

None yet

2 participants