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

Register mce plugin scripts #863

Merged
merged 8 commits into from
May 24, 2017
Merged

Register mce plugin scripts #863

merged 8 commits into from
May 24, 2017

Conversation

ellatrix
Copy link
Member

TO DO: save with more than just the file name.

@ellatrix ellatrix requested a review from nylen May 22, 2017 13:27
@ellatrix
Copy link
Member Author

ellatrix commented May 22, 2017

Something strange is going on with the main tinymce script, I think in master too. It always pops up and is then immediately removed again.

@aduth
Copy link
Member

aduth commented May 22, 2017

I saw an error about the missing lists script in my console this morning too. Does it work when loaded from the CDN? Curious if the script is varying plugin import logic by whether it's loaded off the filesystem.

@ellatrix
Copy link
Member Author

@aduth That's the issue this PR fixes. When it's already loaded, tinymce will use that and not try to fetch the script itself. But I'm seeing something strange with the main script now.

@aduth
Copy link
Member

aduth commented May 22, 2017

My question was more of curiosity, maybe to a broader point of: are we going to encounter other resources TinyMCE will try to load by inferred path because it's assuming we have all scripts in the filesystem? Perhaps not.

@ellatrix
Copy link
Member Author

are we going to encounter other resources TinyMCE will try to load by inferred path because it's assuming we have all scripts in the filesystem?

Not if we don't add any plugins or themes. If we do, we should register it. If a plugin does, they'll need to include it. Same for plugins and core currently.

@mtias mtias added Framework Issues related to broader framework topics, especially as it relates to javascript [Component] TinyMCE labels May 22, 2017
Copy link
Member

@nylen nylen left a comment

Choose a reason for hiding this comment

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

I was a bit concerned about long paths here, due to potential Windows issues (example) and general non-human-friendliness. Addressed in the most recent few commits; I also added a constant to skip downloading these scripts during PHPUnit test runs because this is a bit slow.

Results with SCRIPT_DEBUG = true:

✓ james@xps15 <gutenberg> $ ls vendor/
react.3a8927d9.js      react-dom-server.e84b9fa6.js  tinymce-plugin-lists.557da104.js
react-dom.e5cfcfc9.js  tinymce.c847083c.js

Results with SCRIPT_DEBUG = false:

✓ james@xps15 <gutenberg> $ ls vendor/
react-dom.min.912dc6cf.js         react.min.c7397a88.js    tinymce-plugin-lists.min.e95f0005.js
react-dom-server.min.538e07c8.js  tinymce.min.04a1d751.js

@nylen
Copy link
Member

nylen commented May 23, 2017

Any objections to me merging this in the next day or so? It's a nice bugfix and a speed improvement to the PHPUnit build.

@ellatrix
Copy link
Member Author

No objections really, I'm just finding it getting a lot of code to cache nightly scripts. :)

@nylen
Copy link
Member

nylen commented May 24, 2017

I'm not opposed to ripping it out later. For now we need it, though (it helps :alot: with slow/unreliable connections), and I'm thinking maybe we can re-use some of this machinery for release versions for example.

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.

4 participants