Add manifest path support to conditional reporter #206
+313
−54
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| const getPackages = require('get-monorepo-packages'); | ||
| const isInside = require('path-is-inside'); | ||
| const minimatch = require('minimatch'); | ||
| const _path = require('path'); |
There was a problem hiding this comment.
Why is it underscored?
There was a problem hiding this comment.
I believe this was because the original creator wanted to use path as a variable 😅 .
I might just refactor the plugin to address the issues you mentioned
| '' + | ||
| pkg.package.name + | ||
| (subPackagePath !== '.' ? '/' + subPackagePath : ''); | ||
| const path = _path.parse(basePath).dir; |
| message: `Import for monorepo package '${pkg.package.name}' should be absolute.`, | ||
| fix: (fixer) => { | ||
| const basePath = | ||
| '' + |
| return fixer.replaceText( | ||
| node, | ||
| "'" + | ||
| (name !== '.' && name !== 'index' ? path + '/' + name : path) + |
There was a problem hiding this comment.
Suggested change
| (name !== '.' && name !== 'index' ? path + '/' + name : path) + | |
| (name !== '.' && name !== 'index' ? `${path}/${name}` : path) + |
| |}; | ||
|
|
||
| // Exported for testing | ||
| export async function getConfig({ |
There was a problem hiding this comment.
/nit this can just be in its own file, then you don't need the comment and you can just read the reporter logic without worrying about config at a glance
| logger: PluginLogger, | ||
| tracer: PluginTracer, | ||
| |}): Async<void> { | ||
| const {filename} = await getConfig(options); |
There was a problem hiding this comment.
This should only be retrieved on buildSuccess, otherwise it will slow down builds since reporter events are emitted all the time. Therefore, you can just have an early return when it's not a success event
packages/reporters/conditional-manifest/src/ConditionalManifestReporter.js
Show resolved
Hide resolved
JakeLane
force-pushed
the
add-manifest-path-config-import-cond
branch
from
4ce44f6
to
2bf59de
Compare
JakeLane
force-pushed
the
add-manifest-path-config-import-cond
branch
from
eaf7f43
to
3382ddf
Compare
MonicaOlejniczak
approved these changes
Nov 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Add a configuration option to
package.jsonso we can configure the path for the manifest. This is important as jira needs a separate directory for non-hashed file paths.Changes
Checklist