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

feat(jest-haste-map): support dependencyExtractor written in ESM #12008

Merged
merged 9 commits into from
Feb 24, 2022

Conversation

chentsulin
Copy link
Contributor

Summary

Part of #11167.

See the discussion on the dependencyExtractor config: #11167 (comment)

Test plan

  • Integration test added.
  • Converted new HasteMap(...) calls to await HasteMap.create(...)
  • Converted Runtime.createHasteMap(...) calls to await Runtime.createHasteMap(...)

@chentsulin chentsulin force-pushed the import-dependencyExtractor branch from 5c21bae to db74246 Compare October 28, 2021 09:26
@@ -230,12 +231,16 @@ export default class HasteMap extends EventEmitter {
return HasteMap;
}

static create(options: Options): HasteMap {
static async create(options: Options): Promise<HasteMap> {
Copy link
Member

Choose a reason for hiding this comment

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

code in general looks good, but this makes it a breaking change and must wait for Jest 28 🙂 I'll add it to the milestone so we don't forget

@SimenB SimenB added this to the Jest 28 milestone Oct 28, 2021
@chentsulin chentsulin force-pushed the import-dependencyExtractor branch 3 times, most recently from 8cce621 to f59420e Compare October 28, 2021 11:36
'a.js': 'module.exports = {foo: 5};',
'dependencyExtractor.mjs': `
const DYNAMIC_IMPORT_RE = /(?:^|[^.]\\s*)(\\bdynamicImport\\s*?\\(\\s*?)([\`'"])([^\`'"]+)(\\2\\s*?\\))/g;
export function extract(code, filePath) {
Copy link
Contributor Author

@chentsulin chentsulin Oct 28, 2021

Choose a reason for hiding this comment

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

I am not quite sure we should accept extract named export or a default export here.

named export:

export function extract(code, filePath) {
  const dependencies = new Set();
  if (filePath.includes('skip-deps')) {
    return dependencies;
  }
  const addDependency = (match, pre, quot, dep, post) => {
    dependencies.add(dep);
    return match;
  };
  code.replace(DYNAMIC_IMPORT_RE, addDependency);
  return dependencies;
};

default export:

export default {
  extract(code, filePath) {
    const dependencies = new Set();
    if (filePath.includes('skip-deps')) {
      return dependencies;
    }
    const addDependency = (match, pre, quot, dep, post) => {
      dependencies.add(dep);
      return match;
    };
    code.replace(DYNAMIC_IMPORT_RE, addDependency);
    return dependencies;
  }
}

@chentsulin chentsulin force-pushed the import-dependencyExtractor branch 2 times, most recently from 2030f21 to 7c30268 Compare October 30, 2021 14:15
@SimenB
Copy link
Member

SimenB commented Feb 10, 2022

Hey! I've started to land breaking changes, so if you rebase this we can see where we're at? 🙂

@chentsulin chentsulin force-pushed the import-dependencyExtractor branch from 7c30268 to 08e4923 Compare February 17, 2022 15:32
@chentsulin
Copy link
Contributor Author

Ok. I've done the rebase.

@SimenB
Copy link
Member

SimenB commented Feb 17, 2022

Awesome, thanks! I think I've commented on the two open questions?

@chentsulin chentsulin force-pushed the import-dependencyExtractor branch 2 times, most recently from e9abd9f to e2325bc Compare February 23, 2022 15:58
@chentsulin chentsulin force-pushed the import-dependencyExtractor branch from e2325bc to 6a57b81 Compare February 23, 2022 16:00
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
@chentsulin chentsulin force-pushed the import-dependencyExtractor branch from e4de85a to 55cd8e1 Compare February 23, 2022 16:52
@@ -116,6 +116,56 @@ describe('--findRelatedTests flag', () => {
expect(stderr).toMatch(summaryMsg);
});

test('runs tests related to filename with a custom dependency extractor written in ESM', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SimenB I am not sure if this is a valid e2e test of ESM dependency extractor. I just found a commonjs version above and did the same against ESM.

Copy link
Member

Choose a reason for hiding this comment

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

that's fine 👍

@chentsulin chentsulin requested a review from SimenB February 24, 2022 16:09
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

thanks!

@SimenB SimenB changed the title feat(jest-haste-map): support dependencyExtractor written in ESM feat(jest-haste-map): support dependencyExtractor written in ESM Feb 24, 2022
@SimenB SimenB merged commit ee31422 into jestjs:main Feb 24, 2022
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants