1643696 - isJSON should be renamed to parseJSON

Status: RESOLVED DUPLICATE of bug 1635835

(DevTools :: Netmonitor, task, P3)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

This is a follow up for bug 1635835

isJSON functions should be renamed to parseJSON and it should be used instead of JSON.parse in Network panel code base.

isJSON is defined here:
https://searchfox.org/mozilla-central/rev/598e50d2c3cd81cd616654f16af811adceb08f9f/devtools/client/netmonitor/src/utils/request-utils.js#636

Honza

Comment 1

[Dev Singh]

@Honza please assign this to me as I have already taken care of the rename. I am unable to import this function in other files not in the overall netmonitor folder for some reason, I get the following error:

http://requirejs.org/docs/errors.html#scripterror```

If you have any ideas on how to address this, please let me know.

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for helping with this one!

The problem is that all JSON Viewer files have to use AMD (asynchronous module definition) using

define(function(require, exports, module) {
})

See e.g.
https://searchfox.org/mozilla-central/rev/a87a1c3b543475276e6d57a7a80cb02f3e42b6ed/devtools/client/jsonview/components/Headers.js#7

It's consumed by RequireJS which is loading all the modules.

Can you attach your patch? I can take a look and suggest someting.

Honza

Comment 3

[Dev Singh]

For example, in my diff https://phabricator.services.mozilla.com/D80727 and https://phabricator.services.mozilla.com/D80720, if I attempt to do:
const parseJSON = require("devtools/client/netmonitor/src/utils/request-utils");
and then call the parseJSON function, i get the require error.

Do I need to design the parseJSON function to return a promise instead of simply returning the value? As you can see, simply copy-pasting the function into the JSON viewer file allows it run correctly, but that's obviously not ideal.

Comment 4

[Dev Singh]

If someone could also set this bug to depend on Bug 1642764 (I'm not sure if I can do that), that would be much appreciated. The patches for both bugs 1642764 and 1635835 will implement the fix for this bug.

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Dev Singh from comment #3)

For example, in my diff https://phabricator.services.mozilla.com/D80727 and https://phabricator.services.mozilla.com/D80720, if I attempt to do:
const parseJSON = require("devtools/client/netmonitor/src/utils/request-utils");
and then call the parseJSON function, i get the require error.

Do I need to design the parseJSON function to return a promise instead of simply returning the value? As you can see, simply copy-pasting the function into the JSON viewer file allows it run correctly, but that's obviously not ideal.

Please see my comment here:
https://phabricator.services.mozilla.com/D80727#2468257
Does it make more sense?

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Dev Singh: we can close this bug, correct?

The renaming happens in this patch:
https://phabricator.services.mozilla.com/D80720

Honza

Comment 7

[Dev Singh]

That's correct