Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Require PathUtils instead of using the global one#12203

Merged
marcelgerber merged 1 commit intoadobe:masterfrom
ficristo:require-path-utils
May 2, 2016
Merged

Require PathUtils instead of using the global one#12203
marcelgerber merged 1 commit intoadobe:masterfrom
ficristo:require-path-utils

Conversation

@ficristo
Copy link
Copy Markdown
Collaborator

@ficristo ficristo commented Feb 6, 2016

Actually I decided to try again: similarly to #11734 but this time using the submodule.

MiguelCastillo
MiguelCastillo reviewed Feb 13, 2016
View reviewed changes
Comment thread src/brackets.js
// interim.
var PathUtils = require("thirdparty/path-utils/path-utils");

Object.defineProperty(window, "PathUtils", {
Copy link
Copy Markdown
Contributor

@MiguelCastillo MiguelCastillo Feb 13, 2016

Choose a reason for hiding this comment

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

I like this. Thank you!

@MiguelCastillo
Copy link
Copy Markdown
Contributor

MiguelCastillo commented Feb 13, 2016

These changes look good. When we add breaking changes, we tend to notify extension authors. @marcelgerber, do you think you can help here please?

@petetnt petetnt mentioned this pull request Feb 21, 2016
Open
@marcelgerber
Copy link
Copy Markdown
Contributor

marcelgerber commented Apr 30, 2016

There are quite a few extensions that still use the global object:

  • aem-sightly-brackets-extension
  • brackets-latex
  • cf10.cfbrackets.org
  • cf11.cfbrackets.org
  • cf7.cfbrackets.org
  • cf8.cfbrackets.org
  • cf9.cfbrackets.org
  • luceebrackets.org
  • nicolo-ribaudo.brackets-zip
  • obd3.cfbrackets.org
  • railo2.cfbrackets.org
  • railo3.cfbrackets.org
  • railo4.cfbrackets.org

But, as I see this, this is not a breaking change, as the global object is still accessible, it just outputs an deprecation warning when used. (Am I right?)

@ficristo ficristo force-pushed the require-path-utils branch from a866e9b to c765485 Compare May 1, 2016 15:04
@ficristo
Copy link
Copy Markdown
Collaborator Author

ficristo commented May 1, 2016

Yes, this PR come with a deprecation warning, it shouldn't break extensions.

@marcelgerber
Copy link
Copy Markdown
Contributor

marcelgerber commented May 1, 2016

Same as the Mustache one, this one looks fine. Just that one thing that you could remove the Gruntfile line goes for this, too.

@ficristo ficristo force-pushed the require-path-utils branch from c765485 to 7c544ed Compare May 1, 2016 17:51
@ficristo
Copy link
Copy Markdown
Collaborator Author

ficristo commented May 1, 2016

@marcelgerber done.

@marcelgerber
Copy link
Copy Markdown
Contributor

marcelgerber commented May 1, 2016

Ah, your Mustache changes don't go well together with this one.

@ficristo ficristo force-pushed the require-path-utils branch from 7c544ed to 7dcc5bd Compare May 1, 2016 20:36
@ficristo
Copy link
Copy Markdown
Collaborator Author

ficristo commented May 1, 2016

Fixed (I hope).

@marcelgerber
Copy link
Copy Markdown
Contributor

marcelgerber commented May 2, 2016

Looks good now. Thank you!

@marcelgerber marcelgerber merged commit 81375bb into adobe:master May 2, 2016
@ficristo ficristo deleted the require-path-utils branch May 2, 2016 12:40
marcelgerber pushed a commit that referenced this pull request May 19, 2016
Fix Extension Manager test failures
ce12a86 
Caused by interference between #12389 and #12203.
The former utilizes PathUtils, while the latter adapted PathUtils
to our normal require() workflow.
@core-ai-bot core-ai-bot mentioned this pull request Aug 30, 2021
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ficristo @MiguelCastillo @marcelgerber