Skip to content

fix: handle importing custom helper nicely - #3691

Merged
DavertMik merged 2 commits into
3.xfrom
handle-exported-custom-class-nicely
Jul 2, 2023
Merged

fix: handle importing custom helper nicely#3691
DavertMik merged 2 commits into
3.xfrom
handle-exported-custom-class-nicely

Conversation

@kobenguyent

@kobenguyent kobenguyent commented Jun 7, 2023

Copy link
Copy Markdown
Collaborator

Motivation/Description of the PR

  • This PR aims to handle the importing helpers nicely.
// new export syntax

class CustomHelper {}

export default CustomHelper;


// old export syntax

class CustomHelper {}

export = CustomHelper;

// js class
class TestHelper extends Helper {

}

module.exports = TestHelper;

Type of change

  • 🐛 Bug fix

Checklist:

  • Tests have been added
  • Documentation has been added (Run npm run docs)
  • Lint checking (Run npm run lint)
  • Local tests are passed (Run npm test)

Comment thread lib/container.js
}
const HelperClass = require(moduleName);

// @ts-ignore

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we avoid of the // @ts-ignore by something like this:

const getModule = (moduleName) => {
  const module = require(moduleName);
  return module.default ? module : { default: module };
};

const isHelperModule = moduleName.startsWith('./helper/');

const HelperClass = isHelperModule
  ? require(moduleName)
  : getModule(moduleName).default;

?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Screenshot 2023-06-07 at 16 14 41
it doesn't resolves the issue unfortunately.

@EgorBodnar EgorBodnar Jun 7, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh... I see. Hard situation.

A good time to think about switching to TypeScript and solve the typing issue the correct way 🙂

@DavertMik

Copy link
Copy Markdown
Contributor

Sorry, I can't see purpose of this

export default looks nicer, indeed. But as we still use it through require() I don't see any point of it
It might confuse users as they may think that they can use import inside CodeceptJS which is not true

Moving away from CommonJS would be a nice idea but I see that it requires a lot of time, especially as we have dynamic imports everywhere.

@kobenguyent

kobenguyent commented Jun 9, 2023

Copy link
Copy Markdown
Collaborator Author

Hey @DavertMik, I wanted to discuss a change regarding how we handle the loading of custom helpers. Nowadays, most users are using TypeScript for their custom helpers, and the "export =" syntax, which is deprecated, is being replaced with "export default" to export classes. However, this change causes errors when users try to do the same with CodeceptJS. This pull request aims to address this issue by ensuring that any syntax used to export classes in custom helpers will not cause any problems. By doing so, we won't limit users in their custom helper choices. Of course, it's important to note that their custom helpers will remain their responsibility, but our core functionality will continue to work flawlessly. This change could be a way to attract more users.

@kobenguyent kobenguyent added the typescript Changes for typings or typescript compatibility label Jun 20, 2023

@DavertMik DavertMik left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OK, I'm convinced to accept this
But please do a minor change to improve the code readability

Comment thread lib/container.js Outdated
@DavertMik
DavertMik merged commit 49d1cf6 into 3.x Jul 2, 2023
@DavertMik
DavertMik deleted the handle-exported-custom-class-nicely branch July 2, 2023 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

typescript Changes for typings or typescript compatibility

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants