-
-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
Add lint rule to disallow require.ensure and System.import #1536
Conversation
CI build has failed because the |
You can rebase onto #1538 if you'd like, CI should pass after. |
I'm not sure how to do it. I'll rebase this with master after #1538 is merged. |
No problem. Thanks for the PR! |
Hi @tharakawj! #1538 has been merged, could you please rebase this? 😄 |
@Timer Done! |
The CI log says:
I'll just restart the build. |
It passed! Thanks @gaearon |
'no-restricted-properties': ['warn', { | ||
object: 'require', | ||
property: 'ensure', | ||
message: 'Please use import() instead.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should link to an explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People seeing this warning might not even be aware import()
exists. We should link to some section that clearly explains how to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe we have docs for this yet. 🙈
@tharakawj, would you like to contribute some? 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One pitfall is we won't want to merge that doc until 0.10 or we'll get tens of issues claiming the feature doesn't work. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add the link to the PR's changes/document for now and once it's merged switch it. 🤷♀️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to get this PR in as is if you create an umbrella issue for 0.10 mentioning adding the link (and doc) as todo items.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Timer yeah. I would love to. 😄 I can work on the doc. If you like we can add the link to webpack import() documentation for the time being
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A link to https://webpack.js.org/guides/code-splitting-import/#dynamic-import sounds good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I'll update the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
@gaearon should we still allow webpack to parse |
I'm fine with this being a hard error. |
Unrelated, but how does #1538 work inside Jest? |
@Timer Good point! I'll update the PR |
It doesn't, oops! But #1615 fixes that. Thanks @tharakawj! |
We can get this in? |
Yup, I didn't catch the switch to |
Fixes #1524
Also, disallow
System.import
as it has been deprecated in webpack 2.