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

Replace wp globals with module equivalents #742

Closed
wants to merge 2 commits into from

Conversation

aduth
Copy link
Member

@aduth aduth commented May 9, 2017

Previously: #716, #736, #737, #739

This pull request expands upon the restructuring outlined in #716, replacing wp. global calls with their "WordPress modules" equivalents. It should be the final step in this process.

Testing instructions:

Ensure the build completes, tests pass, and editor initializes and appears without regressions. Specifically observe that no errors are logged to the console, blocks appear as registered with localized (likely English) labels intact.

@aduth aduth added the Framework Issues related to broader framework topics, especially as it relates to javascript label May 9, 2017
@aduth aduth requested a review from mtias May 9, 2017 20:44
aduth added 2 commits May 10, 2017 16:21
Technically available, but treat as error in linting context to avoid
temptation
@aduth aduth force-pushed the update/replace-globals branch from f651e03 to 249ccfc Compare May 10, 2017 20:24
@aduth aduth requested a review from youknowriad May 10, 2017 20:28
@aduth
Copy link
Member Author

aduth commented May 10, 2017

Rebased to resolve conflicts, include new additions, and also remove wp as a whitelisted global so access attempts in code throw lint errors (249ccfc).

I discovered in the process that we may also want to change our React pragma from wp.element.createElement to createElement. This has the downside of meaning we'd now need to import { createElement } from 'element'; in every single file using JSX. I'm going to reserve this for a future pull request.

@youknowriad
Copy link
Contributor

import { createElement } from 'element'; in every single file using JSX

@aduth I think this is good thing, it makes things more explicit.

I think you should rebase and merge this branch right after, because you'll always have conflicts :)

@nylen
Copy link
Member

nylen commented May 15, 2017

Let's just change this to import React from 'react' already.

Edit: though I agree that should be a future PR.

@nylen
Copy link
Member

nylen commented May 30, 2017

@aduth I think we should proceed with this, though arguably it would be easier to just start over. I can help if you like. Do you have concerns with the approach here?

@aduth
Copy link
Member Author

aduth commented May 31, 2017

@nylen

Yes, we should probably do this, and it's probably easier to start over (essentially a find-replace on wp. + import).

One main concern is other wp globals like wp.api.models.Post. How do we handle these?

@nylen
Copy link
Member

nylen commented May 31, 2017

Good question... also seems related to #941.

@aduth
Copy link
Member Author

aduth commented Jun 21, 2017

Superseded by #1332

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants