-
Notifications
You must be signed in to change notification settings - Fork 286
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
chore(esm): change imports to esm compatible #3018
Conversation
- 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. Signed-off-by: Michal Bajer <[email protected]> feat(wip): additional import fixes
[skip ci]
9083cdb
to
f7b553d
Compare
f7b553d
to
fae7518
Compare
@outSH One thing I found out is that you need to run yarn install to update the lockfile because right now 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: 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! |
@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 explained as much here as well hoping that the maintainers of that repo agree. 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 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:
// EDIT Actually, it seem that changing to
|
@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. My pull request: |
@outSH My pull request to add the feature to the paths-filter action is still pending unfortunately. |
@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 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 |
@outSH Thank you! |
.js
suffix required by the ESM standard.Pull Request Requirements
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.-s
flag when usinggit commit
command. You may refer to this link for more information.Character Limit
A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.