feat(core/defaults): Add core/defaults

[pradeepgangwar]

Adds core defaults to respec

Fixes #2023

[pradeepgangwar]

@marcoscaceres I have added some of the defaults on basis of this file. Since I am new to this project let me know if i missed something.
Also from where I can get to know more default conf that can be added?

[marcoscaceres]

This is good @pradeepgangwar. Now, notice that this duplicates things in w3c/defaults - so things that are imported or set to default here shouldn't be w3c/defaults.

That should allow us to start answering the question "from where I can get to know more default conf that can be added?"

We might then need to go and look at a few specs to get a sense of what people are using/enabling.

[pradeepgangwar]

@marcoscaceres This means that If I copy defaults from w3c/defaults to core/defaults then I should remove them from w3c/defaults? Because they are already covered as core defaults.

[marcoscaceres]

This means that If I copy defaults from w3c/defaults to core/defaults then I should remove them from w3c/defaults? Because they are already covered as core defaults.

Precisely. Thanks for confirming.

[pradeepgangwar]

@marcoscaceres I did some changes and pushed the code again. Please review. :)

We might then need to go and look at a few specs to get a sense of what people are using/enabling.

Also please let me know what do i need to do for this. ^

[marcoscaceres]

Also please let me know what do i need to do for this. ^

Let's get the foundational stuff done first, then we can do the above in a separate PR. The above will require us to go look at quite a few specs.

[pradeepgangwar]

Let's get the foundational stuff done first, then we can do the above in a separate PR. The above will require us to go look at quite a few specs.

Sure, Thanks @marcoscaceres . Let me know if this PR requires further changes. :)

[marcoscaceres]

Looking really good, @pradeepgangwar! Just a couple of little things.

[marcoscaceres]

@pradeepgangwar, could you please revert the changes to the files in the "build/" directory? We only build as part of a release.

[marcoscaceres]

@saschanaz, would really appreciate your input on this one. Do you think we should keep the current defaults, or turn everything off by default in "core/defaults"?

[saschanaz]

Do you think we should keep the current defaults, or turn everything off by default in "core/defaults"?

Keeping the current sounds okay to me if they are recommended.

[marcoscaceres]

Keeping the current sounds okay to me if they are recommended.

Ok, just wanted to double check I wasn't overlooking something obvious. Let's run with this current structure.

[marcoscaceres]

@pradeepgangwar, two tests are failing:

    ✖ sets sensible defaults for w3c specs
    ✖ allows w3c defaults to be overridden

To figure out why, you will need to have a look at:
https://github.com/w3c/respec/blob/develop/tests/spec/w3c/defaults-spec.js

And to run the tests locally:

npm start

And navigate to:

http://localhost:5000/tests/SpecRunner.html

You should then be able to debug the tests that are failing (e.g., by setting a debugger statement in the test to figure out what's different).

[pradeepgangwar]

@marcoscaceres Looks like it is not taking into account the core/defaults while determining defaults for w3c in test file. Earlier it was working when I imported core/defaults in w3c/defaults. Is it like that core/defaults are automatically set and w3c/defaults overrides that.

Debugging statements show that w3c/defaults didn't had core/defaults in tests.

[saschanaz]

Looks like we created core/defaults but not using it at all, with no imports for the module.

[marcoscaceres]

Looks like we created core/defaults but not using it at all, with no imports for the module.

lol, yeah - you are right @saschanaz ... sorry @pradeepgangwar, I should spotted that.

As this is "core", you need to import the "core/defaults" plugin directly into base-runner.js.

Then you should unshift() the "core/defaults" into the plugs array here:
https://github.com/w3c/respec/blob/develop/src/core/base-runner.js#L53

[saschanaz]

Then you should unshift() the "core/defaults" into the plugs array here:

Or we may just explicitly call run() there. (I would change the function name if we go this way.)

[marcoscaceres]

Or we may just explicitly call run() there. (I would change the function name if we go this way.)

I think we should keep treating it as a plugin, just to be consistent. If we ever update it to be async, the base runner will know how to deal with it. Same with errors, and measuring perf stuff and so on. That will all work the same as with other plugins.

[saschanaz]

Same with errors, and measuring perf stuff and so on. That will all work the same as with other plugins.

I think we somehow should refactor the current plugin system. We know our core plugins are dependent on others but it's currently hard to figure out which depends on which. Importing and calling module functions explicitly may help here.

That said, maybe it's okay to keep consistency in this PR.

[marcoscaceres]

🤞 fingers crossed that fixed it! 🤞

[pradeepgangwar]

Now some new tests are failing. 😢 I am not sure why they are failing.

[marcoscaceres]

Failing tests is a good thing: it means that the new defaults are being applied :) We are changing some pretty fundamental stuff in ReSpec, so some tests are going to get upset.

[marcoscaceres]

@pradeepgangwar, are you good to investigate these failures or would you prefer I have a look? Understand if it's starting to get more "in the weeds" than originally intended. However, if you want a bit of a challenge, it might be worth while seeing why those two tests are failing.

[pradeepgangwar]

@marcoscaceres Sure I wil try to investigate the tests. Give me some time ... Maybe till tomorrow. I will let you know If I get into some trouble.

[marcoscaceres]

Sounds good @pradeepgangwar. Appreciate your help.

[pradeepgangwar]

@marcoscaceres I tried it for but could not figure out actual problem. 🤕 Can you help me here.

[marcoscaceres]

Will try to have a look soon.

[saschanaz]

Okay, I found what's the problem for the test "resolve with the resulting respecConfig". Previously the code did not overwrite conf.lint if it's not an object, as it possibly can be false and Object.assign(conf.lint, { ... }) does nothing then. Now we always rewrite conf.lint as object which caused the test failure.

[saschanaz]

The problem with the test "uses inline references to provide context" is similar. We now overwrites on lint: false so it forcibly enabled all linters, which caused the warning count to be different.

[pradeepgangwar]

@marcoscaceres @saschanaz Thanks a lot! Tests passed. 😃

[marcoscaceres]

Thanks all for your persistence on this. I think this moves us in a good new direction.