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

Replaces deprecated methods #6244

Closed

Conversation

MartinM85
Copy link
Contributor

Closes #6237

@MartinM85
Copy link
Contributor Author

Hi @milanholemans, could you please re-run the workflow? Seems like a random error. Thanks

@milanholemans
Copy link
Contributor

Thank you @MartinM85, we'll try to review it ASAP.

@MartinM85
Copy link
Contributor Author

MartinM85 commented Aug 14, 2024

Not sure right now, but the error on macos

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory

Can be related to eslint-plugin-deprecation
gund/eslint-plugin-deprecation#67

@milanholemans
Copy link
Contributor

Not sure right now, but the error on macos

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory

Can be related to eslint-plugin-deprecation gund/eslint-plugin-deprecation#67

Hmm that's quite problematic.

@milanholemans
Copy link
Contributor

If it's failing our pipelines, and we can't fix it, we should consider something else.

@milanholemans milanholemans marked this pull request as draft August 14, 2024 21:18
@MartinM85
Copy link
Contributor Author

I have also encountered an issue with increased execution time of the linting. The CLI for M365 has a large code base, the performance of this plugin is really bad.

In the worst case, we can get rid of the plugin eslint-plugin-deprecation and keep the refactored code.

@milanholemans
Copy link
Contributor

I have also encountered an issue with increased execution time of the linting. The CLI for M365 has a large code base, the performance of this plugin is really bad.

In the worst case, we can get rid of the plugin eslint-plugin-deprecation and keep the refactored code.

That's a pity. Like you mentioned, if we can't get it to work, we'll just review the refactored code.

@MartinM85 MartinM85 marked this pull request as ready for review August 15, 2024 11:41
@MartinM85
Copy link
Contributor Author

Plugin eslint-plugin-deprecation removed

@MartinM85 MartinM85 force-pushed the feature/6237-eslint-rule-deprecation branch from 974af73 to 2b34143 Compare September 13, 2024 12:11
Copy link
Member

@waldekmastykarz waldekmastykarz left a comment

Choose a reason for hiding this comment

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

Could it be that we're missing changes to .eslintrc.cjs where we enable the rule?

@MartinM85
Copy link
Contributor Author

@waldekmastykarz eslint-plugin-deprecation was causing the error when running test on macos.

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory

Similar to gund/eslint-plugin-deprecation#67

https://github.com/pnp/cli-microsoft365/actions/runs/10370949281/job/28746774555

and increased execution time of the linting, because the CLI for M365 has a large code base, the performance of this plugin is really bad.

I only refactored deprecated functions.

@waldekmastykarz
Copy link
Member

Thanks for clarifying. Let's update the PR's title to communicate this more clearly.

@waldekmastykarz waldekmastykarz changed the title Enhancement: Deprecation eslint rule. Closes #6237 Replaces deprecated methods Sep 15, 2024
src/AuthServer.ts Outdated Show resolved Hide resolved
@milanholemans milanholemans self-assigned this Nov 10, 2024
Copy link
Contributor

@milanholemans milanholemans left a comment

Choose a reason for hiding this comment

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

Nice job! The code is looking a lot better. I have a few comments here and there.

src/AuthServer.ts Outdated Show resolved Hide resolved
src/utils/urlUtil.ts Outdated Show resolved Hide resolved
src/utils/urlUtil.ts Outdated Show resolved Hide resolved
src/utils/urlUtil.ts Outdated Show resolved Hide resolved
src/utils/urlUtil.ts Outdated Show resolved Hide resolved
@milanholemans milanholemans marked this pull request as draft November 10, 2024 23:30
@MartinM85 MartinM85 marked this pull request as ready for review November 11, 2024 07:23
Copy link
Contributor

@milanholemans milanholemans left a comment

Choose a reason for hiding this comment

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

Nothing more to add!

@milanholemans
Copy link
Contributor

Merged manually. Thanks for the fix!

nanddeepn pushed a commit to nanddeepn/cli-microsoft365 that referenced this pull request Nov 25, 2024
SmitaNachan pushed a commit to SmitaNachan/cli-microsoft365 that referenced this pull request Dec 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancement: Deprecation eslint rule
3 participants