cacti icon indicating copy to clipboard operation
cacti copied to clipboard

chore(esm): change imports to esm compatible

Open outSH opened this issue 1 year ago • 4 comments

  • Change all relative path imports to have .js suffix required by the ESM standard.
  • Use full import path instead of directory default import.
  • I've created a tool for doing this automatically (existing tools were failing on some edge cases). The sources are available here: https://github.com/outSH/to-esm-imports.
  • The tool is executed after codegen (because openapi generators doesn't support creating ESM-compatible imports yet).
  • Changed jest and webpack configs to work with fully qualified ESM imports.

Pull Request Requirements

  • [ ] Rebased onto upstream/main branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.
  • [ ] Have git sign off at the end of commit message to avoid being marked red. You can add -s flag when using git commit command. You may refer to this link for more information.
  • [ ] Follow the Commit Linting specification. You may refer to this link for more information.

Character Limit

  • [ ] Pull Request Title and Commit Subject must not exceed 72 characters (including spaces and special characters).
  • [ ] Commit Message per line must not exceed 80 characters (including spaces and special characters).

A Must Read for Beginners For rebasing and squashing, here's a must read guide for beginners.

outSH avatar Feb 09 '24 17:02 outSH

@outSH One thing I found out is that you need to run yarn install to update the lockfile because right now build-dev is failing and that just marks (almost) everything as skipped due to the dependency on build-dev.

With that said, I fixed this on my own branch and it's still not working as expected. My other suspicion that I'm looking into as we speak is this line of code in the path-filter library that explicitly excludes file renames from the list of changed files: https://github.com/dorny/paths-filter/blob/master/src/git.ts#L30

Not sure if this is what is causing it but I wanted to keep you in the loop about it either way. More to come!

image

petermetz avatar Feb 21 '24 16:02 petermetz

@outSH OK, a slightly different place in the code but with the same technical decision by the maintainer to not include renames: https://github.com/dorny/paths-filter/blob/master/src/main.ts#L185-L187

My own take: I disagree with renames not making sense to include as changes because if I move files around and forget to update the imports then I had just broken the build which the CI should definitely catch.

I explained as much here as well hoping that the maintainers of that repo agree. https://github.com/dorny/paths-filter/issues/152#issuecomment-1957351279

Later on if I have enough time I could send a PR myself to that repo to extend it with this functionality but in the meantime I propose that we move ahead as-is.

petermetz avatar Feb 21 '24 17:02 petermetz

@petermetz This occurred when build was working, it's still WIP. Also, there are no file renames here so how does it apply here? I'm still thinkinking this is caused by picomatch limitations, here's sample code:

const pm = require("picomatch");

const isMatch = pm(
  "./packages/cactus-plugin-keychain-vault/**!(*.md|*.css|*.html|*.jpg|*.jpeg|*.png)"
);
console.log(
  "packages/cactus-plugin-keychain-vault/foo",
  isMatch("packages/cactus-plugin-keychain-vault/foo")
);
console.log(
  "packages/cactus-plugin-keychain-vault/foo.md",
  isMatch("packages/cactus-plugin-keychain-vault/foo.md")
);
console.log(
  "packages/cactus-plugin-keychain-vault/foo.html",
  isMatch("packages/cactus-plugin-keychain-vault/foo.html")
);
console.log(
  "packages/cactus-plugin-keychain-vault/foo.jpg",
  isMatch("packages/cactus-plugin-keychain-vault/foo.jpg")
);
console.log(
  "packages/cactus-plugin-keychain-vault/foo.ts",
  isMatch("packages/cactus-plugin-keychain-vault/foo.ts")
);
console.log(
  "packages/cactus-plugin-keychain-vault/foo.asd",
  isMatch("packages/cactus-plugin-keychain-vault/foo.asd")
);

console.log(
  "packages/cactus-plugin-keychain-vault/src/main/typescript/plugin-keychain-vault.ts",
  isMatch(
    "packages/cactus-plugin-keychain-vault/src/main/typescript/plugin-keychain-vault.ts"
  )
);
console.log(
  "packages/cactus-plugin-keychain-vault/src/main/typescript/foo.ts",
  isMatch("packages/cactus-plugin-keychain-vault/src/main/typescript/foo.ts")
);
console.log(
  "packages/cactus-plugin-keychain-vault/src/main/typescript/foo.js",
  isMatch("packages/cactus-plugin-keychain-vault/src/main/typescript/foo.js")
);
console.log(
  "packages/cactus-plugin-keychain-vault/src/main/typescript/foo.png",
  isMatch("packages/cactus-plugin-keychain-vault/src/main/typescript/foo.png")
);
console.log(
  "packages/cactus-plugin-keychain-vault/src/main/typescript/foo.jpg",
  isMatch("packages/cactus-plugin-keychain-vault/src/main/typescript/foo.jpg")
);
console.log(
  "packages/cactus-plugin-keychain-vault/src/main/typescript/foo.md",
  isMatch("packages/cactus-plugin-keychain-vault/src/main/typescript/foo.md")
);

and output:

packages/cactus-plugin-keychain-vault/foo true
packages/cactus-plugin-keychain-vault/foo.md true
packages/cactus-plugin-keychain-vault/foo.html true
packages/cactus-plugin-keychain-vault/foo.jpg true
packages/cactus-plugin-keychain-vault/foo.ts true
packages/cactus-plugin-keychain-vault/foo.asd true
packages/cactus-plugin-keychain-vault/src/main/typescript/plugin-keychain-vault.ts false
packages/cactus-plugin-keychain-vault/src/main/typescript/foo.ts false
packages/cactus-plugin-keychain-vault/src/main/typescript/foo.js false
packages/cactus-plugin-keychain-vault/src/main/typescript/foo.png false
packages/cactus-plugin-keychain-vault/src/main/typescript/foo.jpg false
packages/cactus-plugin-keychain-vault/src/main/typescript/foo.md false

// EDIT Actually, it seem that changing to "./packages/cactus-plugin-keychain-vault/**/!(*.md|*.css|*.html|*.jpg|*.jpeg|*.png)" seem to work, don't know how I didn't found it earlier on o_O

packages/cactus-plugin-keychain-vault/foo true
packages/cactus-plugin-keychain-vault/foo.md false
packages/cactus-plugin-keychain-vault/foo.html false
packages/cactus-plugin-keychain-vault/foo.jpg false
packages/cactus-plugin-keychain-vault/foo.ts true
packages/cactus-plugin-keychain-vault/foo.asd true
packages/cactus-plugin-keychain-vault/src/main/typescript/plugin-keychain-vault.ts true
packages/cactus-plugin-keychain-vault/src/main/typescript/foo.ts true
packages/cactus-plugin-keychain-vault/src/main/typescript/foo.js true
packages/cactus-plugin-keychain-vault/src/main/typescript/foo.png false
packages/cactus-plugin-keychain-vault/src/main/typescript/foo.jpg false
packages/cactus-plugin-keychain-vault/src/main/typescript/foo.md false

outSH avatar Feb 21 '24 18:02 outSH

@outSH You are a 100% right, sorry for the complications from my investigation. When I did the full deep-dive I narrowed it down to a single line in the code of the paths-filter action that we could update to make our use-case work with slightly differently formatted globs where the exclusion globs are on their own line and we specify that all of the patterns have to match instead of "at least one".

I've submitted a patch to the paths-filter action directly and hoping that the idea will get some traction there and then it will be merged and released so we can use it. If not, then plan B is to just use the fork as long as the original author(s) are OK with that.

My pull request: https://github.com/dorny/paths-filter/pull/224

petermetz avatar Feb 22 '24 20:02 petermetz

@outSH My pull request to add the feature to the paths-filter action is still pending unfortunately. In the meantime, I wanted to check in and ask: Are you still working on this PR or should we close it for now? (I'm trying to declutter the PR queue)

petermetz avatar May 24 '24 21:05 petermetz

@outSH My first PR to the paths-filter library was accepted and merged but there was a bug in it so I had to submit a second one which hasn't been looked at by the maintainer of that package for months now unfortunately. I thought about publishing a friendly fork of the path-filter action but in the meantime Jennifer made some headway in the dynamic diff analysis so we might not need path filtering at all in the near future.

On another note though: In general, are you still working on this PR or should we just put it to rest for now? It's been open for a while and I'm trying real hard to bring the number of pending PRs down.

petermetz avatar Jun 26 '24 20:06 petermetz

On another note though: In general, are you still working on this PR or should we just put it to rest for now? It's been open for a while and I'm trying real hard to bring the number of pending PRs down.

@petermetz I'm not working at this ATM, but this is just a PR draft. Is it displayed on your "to review" list? I wanted to keep it open to find it quicker once I get some time / energy to sit down to it again :)

outSH avatar Jul 01 '24 10:07 outSH

On another note though: In general, are you still working on this PR or should we just put it to rest for now? It's been open for a while and I'm trying real hard to bring the number of pending PRs down.

@petermetz I'm not working at this ATM, but this is just a PR draft. Is it displayed on your "to review" list? I wanted to keep it open to find it quicker once I get some time / energy to sit down to it again :)

@outSH It's not in my review list (so it's totally OK on that front) but the other (kinda) list (of the many) that I have is this one about the project health in terms of the velocity of PR throughput and reviews where we have pretty bad numbers due to PRs being left open on the upstream repo as drafts.

If you are not actively working on it but would still prefer a reminder, maybe I could recommend to open a PR against the main of your own fork? I'm also open to any other idea that would reduce the skew in the statistics linked below (I think we are doing much better in terms of PR velocity this year than last year, BUT a few of these long-running PRs are probably making it look worse than it is).

Source: https://insights.lfx.linuxfoundation.org/foundation/hyp/velocity?project=cacti&routedFrom=Github&bestPractice=false

Screenshot: image

petermetz avatar Jul 03 '24 23:07 petermetz

@outSH Thank you!

petermetz avatar Jul 05 '24 23:07 petermetz