feature: ignore nested node modules #258
Conversation
✅ Deploy Preview for custom-elements-manifest-analyzer ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
package.json
Outdated
| @@ -32,6 +32,7 @@ | |||
| }, | |||
| "workspaces": [ | |||
| "packages/*", | |||
| "packages/analyzer/fixtures/11-nested-node-modules/01-basic", | |||
There was a problem hiding this comment.
This is to generate a nested node_modules folder
There was a problem hiding this comment.
Can we reuse this test fixture instead? https://github.com/open-wc/custom-elements-manifest/blob/master/packages/analyzer/fixtures/02-inheritance/05-external-in-monorepo/monorepo/package.json
There was a problem hiding this comment.
Great, I removed the new test case and used the existing one. Annoyingly, the test case (even after I added a node_modules directory) passes on both the old and the new regex. My previous test failed on the old regex, but passed with the new one.
I can investigate further, but again it would probably complicate the repo structure a bit, what do you think?
There was a problem hiding this comment.
Yeah id prefer not to use the workspaces field for it, it muddies the water a bit
There was a problem hiding this comment.
👍🏽 the latest change removes the changes to the workspaces
The PR's ready for review, but I couldn't quite get the 05-external-in-monorepo test case to fail on the previous regex. It passes on both. I can investigate further, but not sure if it's a blocker
There was a problem hiding this comment.
FYI: I checked in this node_modules directory as the other node_modules directory within this fixture is also checked in.
There was a problem hiding this comment.
I added this folder as the test didn't fail, even when no node_modules ignore regex was passed through
| @@ -1401,19 +1401,6 @@ | |||
| "@babel/helper-module-imports" "^7.10.4" | |||
| "@rollup/pluginutils" "^3.1.0" | |||
|
|
|||
| "@rollup/plugin-commonjs@^19.0.0": | |||
There was a problem hiding this comment.
I don't know why the yarn.lock changed when I installed the dependencies. I can revert these changes if it's a problem
| @@ -4,11 +4,12 @@ | |||
| "type": "module", | |||
| "dependencies": { | |||
| "@mono/sibling-pkg": "0.2.0", | |||
| "ext-pkg-without-export-map": "0.1.0" | |||
| "ext-pkg-without-export-map": "0.1.0", | |||
| "is-sorted": "1.0.5" | |||
There was a problem hiding this comment.
I added this dependency to ensure that the test fails if the regex is not present, it passes otherwise.
|
Heya, just wanted to get your thoughts on this, whether there's anything you'd like me to clarify or change :) |
Note: I added a test case to simulate the bad scenario, but it seems to work even with the old regex.The test case now fails using the old regex pattern, but the set up for the test is complex. I worry that it pollutes the top-level package.json file.
What do you think? Happy to remove the test if you feel it best
Resolves: #256