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

chore(esm): change imports to esm compatible #3018

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

outSH
Copy link
Contributor

@outSH outSH commented Feb 9, 2024

  • 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.

- 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 <michal.bajer@fujitsu.com>

feat(wip): additional import fixes
@petermetz
Copy link
Member

@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
Copy link
Member

@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.
dorny/paths-filter#152 (comment)

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.

@outSH
Copy link
Contributor Author

outSH commented Feb 21, 2024

@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

@petermetz
Copy link
Member

@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:
dorny/paths-filter#224

@petermetz
Copy link
Member

@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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants