You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
This repository was archived by the owner on Sep 6, 2021. It is now read-only.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I too had been contemplating updating the CodeMirror submodule name and its side-effects. Merging the changes.
Have raised an issue to track the action items mentioned in the description post this merge.
We could have delayed this merge till the next release and give a heads-up in this release about the impending change; This could be a breaking change with some extensions. Also the three issues mentioned by marcel had to be verified before this change was merged.
I think we should revert this change for this release to allow extension devs enough time to verify their extensions or alternatively check the impact of this change before merge.
Created a revert just in case we decide to revert this for the release. @prafulVaishnav@nethip
There's one extension that uses CodeMirror2 for literal access: Brackets-Themes by @MiguelCastillo - all the others only use brackets.getModule.
Knowing this, I think we don't need to revert the changes. Miguel is probably pretty fast in updating his extension so it checks whether CodeMirror2 or CodeMirror should be used.
Point 2: We don't really need them (it's a "nice to have" and nothing else) as we can simply leave the requirejs rerouting in.
Point 3: That's just our infrastructure and it's very easy and non-breaking to do.
Thanks Marcel, i'll run a search for the usage of codeMirror2 on the source codes in the extension registry just in case and update. If the extension usage is proper, there would be nothing to worry about this going in the release 🐹
@MiguelCastillo This PR is already merged, but it's not yet part of a Brackets Release.
One other thing you could possibly do is check whether CodeMirror2 still exists and use the appropiate directory based on the result. Just a suggestion, though.
@abose That's actually what I already did, but nothing to say against a second pair of eyes :)
@marcelgerber oh man, I must be going crazy. I could not find the merged comment! I see it now that I went through the thread in more details... I wondering why I couldn't see the merge button :)
@MiguelCastillo Notice though that your PR will only work in Brackets, so better set your package.json's minimum Brackets version to 1.4, too.
That's why I suggested conditionally detecting what path to use above: It will still work for both versions post- and pre-1.4.
In order to make that work, don't yet change the brackets.getModule paths - CodeMirror2 will still work fine for now.
@marcelgerber Thanks for fixing this. This was a very good observation.
And thanks to @MiguelCastillo and @abose for collaborating on making sure this won't effect the other modules.
Since these are all architectural changes I feel we should use our slack channel to bring these up so that everyone can put in their opinions before merging these kind of changes to master.
@marcel, I ran a search on the extension registry for CodeMirro2 and it seems that all of them except Miguel's extension is using Bracket.getModule("path?Codemirror2/file") format. So this change would be safe for this release as you said.
I will go ahead and delete the reverting PR.
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Labels
None yet
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Now that we use CodeMirror 5, it's a little misleading our submodule is still called
CodeMirror2
. Thus, in this PR, I've renamed it toCodeMirror
.There are some things to keep in mind, though:
CodeMirror2
(FileSystem, node) won't work. Look out for those.CodeMirror2
through anything else than arequire
(FileSystem, node, ...)