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

Add lint rule to disallow require.ensure and System.import #1536

Merged
merged 1 commit into from
Feb 24, 2017

Conversation

tharakawj
Copy link
Contributor

Fixes #1524

Also, disallow System.import as it has been deprecated in webpack 2.

@tharakawj
Copy link
Contributor Author

CI build has failed because the kitchensink template for e2e testing uses require.ensure. I think first we need to add support for dynamic import() and replace require.ensure with import().

@Timer Timer added this to the 0.10.0 milestone Feb 12, 2017
@Timer
Copy link
Contributor

Timer commented Feb 12, 2017

You can rebase onto #1538 if you'd like, CI should pass after.

@Timer Timer mentioned this pull request Feb 12, 2017
5 tasks
@tharakawj
Copy link
Contributor Author

tharakawj commented Feb 13, 2017

I'm not sure how to do it. I'll rebase this with master after #1538 is merged.

@Timer
Copy link
Contributor

Timer commented Feb 13, 2017

No problem. Thanks for the PR!

@Timer
Copy link
Contributor

Timer commented Feb 15, 2017

Hi @tharakawj! #1538 has been merged, could you please rebase this? 😄

@tharakawj
Copy link
Contributor Author

@Timer Done!
but the build has failed. Any idea??

@gaearon
Copy link
Contributor

gaearon commented Feb 15, 2017

@tharakawj
Copy link
Contributor Author

It passed! Thanks @gaearon

'no-restricted-properties': ['warn', {
object: 'require',
property: 'ensure',
message: 'Please use import() instead.'
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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? 😄

Copy link
Contributor

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. 😄

Copy link
Contributor

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. 🤷‍♀️

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@Timer
Copy link
Contributor

Timer commented Feb 22, 2017

@gaearon should we still allow webpack to parse require.ensure since this is only set to warning? master has require.ensure disabled -- I don't want webpack to blow up (says cannot statically analyse deps). Alternatively, we could make this lint rule error out.

@gaearon
Copy link
Contributor

gaearon commented Feb 22, 2017

I'm fine with this being a hard error.

@gaearon
Copy link
Contributor

gaearon commented Feb 22, 2017

Unrelated, but how does #1538 work inside Jest?

@tharakawj
Copy link
Contributor Author

@Timer Good point! I'll update the PR

@Timer
Copy link
Contributor

Timer commented Feb 22, 2017

Unrelated, but how does #1538 work inside Jest?

It doesn't, oops! But #1615 fixes that.


Thanks @tharakawj!

@gaearon
Copy link
Contributor

gaearon commented Feb 24, 2017

We can get this in?

@Timer
Copy link
Contributor

Timer commented Feb 24, 2017

Yup, I didn't catch the switch to error.

@gaearon gaearon merged commit 09cfcde into facebook:master Feb 24, 2017
@lock lock bot locked and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants