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

Change import package #2373

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Change import package #2373

wants to merge 8 commits into from

Conversation

jarqvi
Copy link

@jarqvi jarqvi commented Jan 28, 2024

What issue does this pull request resolve?
In fact, some codes that were not needed much have been removed.

What changes did you make?
The recent removal of the line ‍‍‍const {default: Ajv} = require('ajv') was made possible by eliminating the export statement module.exports.default = AjvClass. As a result, we no longer require the named import of the Ajv module.

And since we mentioned the following code sample in the documentation, I don't think there is a problem:
const Ajv = require("ajv")

Is there anything that requires more attention while reviewing?
require packages in CJS

@jarqvi
Copy link
Author

jarqvi commented Jan 29, 2024

Or at least it can be as follows:
const {Ajv} = require('ajv')

The following also works:
const Ajv = require('ajv')

I feel the code is cleaner with this method instead of using the code below:
const {default: Ajv} = require('ajv')

@jarqvi
Copy link
Author

jarqvi commented Feb 24, 2024

@jasoniangreen what happened?!

@jarqvi jarqvi changed the title refactor: remove extra codes Change import package Feb 24, 2024
@jasoniangreen
Copy link
Collaborator

@jasoniangreen what happened?!

I just wanted to pull master into your branch so I can see if it passes and do a proper review in the up to date context.

@jarqvi
Copy link
Author

jarqvi commented Feb 25, 2024

Thanks.

@jasoniangreen
Copy link
Collaborator

Hi @jarqvi, I don't really understand why should make this change?

@jarqvi
Copy link
Author

jarqvi commented Jun 6, 2024

Hi @jasoniangreen,
I felt that it has more readability when importing package.

@jasoniangreen
Copy link
Collaborator

Hi @jasoniangreen, I felt that it has more readability when importing package.

Ah ok, but it would be a potentially breaking change, correct? Depending on how people are currently importing it?

@jarqvi
Copy link
Author

jarqvi commented Jun 17, 2024

Hi @jasoniangreen, I felt that it has more readability when importing package.

Ah ok, but it would be a potentially breaking change, correct? Depending on how people are currently importing it?

Well, we can make these changes by keeping the previous method and be backward compatible

@jarqvi
Copy link
Author

jarqvi commented Jul 13, 2024

Hi @jasoniangreen, I felt that it has more readability when importing package.

Ah ok, but it would be a potentially breaking change, correct? Depending on how people are currently importing it?

Well, we can make these changes by keeping the previous method and be backward compatible

@jasoniangreen Done.

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

Successfully merging this pull request may close these issues.

2 participants