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 ignore path problem #106

Merged
merged 3 commits into from Aug 8, 2023
Merged

Conversation

hl662
Copy link

@hl662 hl662 commented Aug 1, 2023

Other users have had the same issues with copy webpack plugin here. There doesn't seem to be a consensus on what the solution is, and the issue is still open. I went into the files and am attaching the relevant code block here. It shows how globOptions is passed into globby.
Looking at the code block, the plugin constructs a glob Path, and then passes that and the globOptions as is into globby. globby then uses fast-glob to add the globOptions.ignore strings. Over the course of last month, globby and fast-glob both had one PR merged addressing ignore.

globby merged a PR that addressed its default handling of ignore options.
fast-glob merged a PR that addressed misuse of the ignore options.

I started by setting noErrorOnMissing to false and ran pnpm start with USING_NPM set to true. I found 18 errors were raised, all with the same message as this:

ERROR in unable to locate '/Users/{USER_NAME}/Documents/your-app-name/node_modules/.pnpm/@itwin+core-bentley@4.0.6/node_modules/@itwin/core-bentley/**/public/**' glob

When USING_NPM is set to false, the same errors were raised, but 29 this time.
This means our default working solution raises errors that aren't raised because we set noErrorOnMissing to true.
The change I made involved just tweaking the ignore glob we pass in such that the compilation errors went back to 18 errors.

The fix as is will be fine, it will revert the behavior of copy webpack plugin back to what it was. Another solution would be to figure out how to fix the glob pattern that we pass into from. The lines here in the plugin notes how the full glob path is formed (by joining the context with the form glob pattern we pass in).

One last thing to note is copy-webpack-plugin actually hasn't bumped their versions of globby or fast-glob for almost a year....

@aruniverse
Copy link
Member

does this work with npm and yarn and pnpm ?
do we need the USING_NPM flag anymore?
does this work with workspaces / monorepos?

@hl662
Copy link
Author

hl662 commented Aug 8, 2023

does this work with npm and yarn and pnpm ? do we need the USING_NPM flag anymore? does this work with workspaces / monorepos?

we don't need the USING_NPM flag anymore, so I removed the use of them in webpack.config.js. After adding index.html to the globOptions.ignore, this now works within monorepos. npm, yarn, and pnpm all work.

@hl662 hl662 marked this pull request as ready for review August 8, 2023 16:14
Copy link
Member

@aruniverse aruniverse left a comment

Choose a reason for hiding this comment

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

could you update the package.json version? bump the patch version
theres also an readme-imodeljs.md that needs to be updated

@hl662
Copy link
Author

hl662 commented Aug 8, 2023

could you update the package.json version? bump the patch version theres also an readme-imodeljs.md that needs to be updated

done

@aruniverse aruniverse merged commit a9b7a23 into imodeljs:imodeljs Aug 8, 2023
1 of 8 checks passed
@aruniverse aruniverse linked an issue Aug 8, 2023 that may be closed by this pull request
ben-polinsky added a commit to ben-polinsky/create-react-app that referenced this pull request Sep 29, 2023
aruniverse pushed a commit that referenced this pull request Sep 29, 2023
…visitor issue (#108)

* fix leftover issue from #106; bump @babel/core to fix visitor issue

* react-scripts version bump - 5.0.6

* remove now-unnecessary regex escape character

---------

Co-authored-by: Ben Polinsky <ben-polinsky@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

CopyAssetsPlugin broken
2 participants