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.

Sign up for GitHub

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

Jump to bottom

Revert "module: add require.resolve (#445)" #485

Merged
merged 1 commit into from
Jan 16, 2019
Merged

Revert "module: add require.resolve (#445)" #485

merged 1 commit into from
Jan 16, 2019

Conversation

yorkie
Copy link
Member

@yorkie yorkie commented Jan 16, 2019 •
edited
Loading

This reverts commit 7807fe4, that causes the require performance problems.

/cc @legendecas @lolBig

Checklist
  • npm test passes
  • tests and/or benchmarks are included
  • documentation is changed or added

@yorkie
Revert "module: add require.resolve (#445)"
6cb04b6 
This reverts commit 7807fe4.
This causes the `require` performance problems.
@yorkie yorkie added the module label Jan 16, 2019
legendecas
legendecas approved these changes Jan 16, 2019
View reviewed changes
Copy link
Contributor

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

Still, we needs more benckmarks on require since it is a crucial path on ShadowNode.

@yorkie yorkie merged commit a96b7ca into master Jan 16, 2019
@yorkie yorkie deleted the revert/module branch January 16, 2019 13:50
@Txiaozhe
Copy link
Contributor

What scenarios can cause performance problems？

@yorkie yorkie restored the revert/module branch January 16, 2019 13:54
yorkie added a commit that referenced this pull request Jan 16, 2019
@yorkie
Revert "module: add require.resolve (#445)" (#485)
0e905d0 
This reverts commit 7807fe4.
This causes the `require` performance problems.
@yorkie
Copy link
Member Author

yorkie commented Jan 16, 2019 •
edited
Loading

What scenarios can cause performance problems?

I have to say it's all, we just found the time of booting yodart takes slower than 2x.

legendecas pushed a commit that referenced this pull request Jan 16, 2019
@yorkie @legendecas
Revert "module: add require.resolve (#445)" (#485)
25e9fee 
This reverts commit 7807fe4.
This causes the `require` performance problems.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@legendecas legendecas legendecas approved these changes

@Txiaozhe Txiaozhe Awaiting requested review from Txiaozhe

Assignees
No one assigned
Labels
module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yorkie @Txiaozhe @legendecas