Module require resolving refactor #493

dustyo-O · 2017-12-10T12:21:52Z

Several probs with require more than one module in different modular systems

Changes proposed in this pull request

If we have ym - we provide modules there, also we wrap commonJS and global deps with ym syntax
If we do not have ym, we ignore ym deps.
Priority: global - commonJS - ym;

Checklist

Documentation changed
Tests added

dustyo-O · 2017-12-10T12:36:20Z

lib/compiler.js

+              var commonPath = deps.commonJS[dep];
+              return '_libs["' + dep + '"] = ' +
+                'glob && typeof glob["' + dep + '"] ' +
+                '!== "undefined" ? ' +


looks like no need to do it (try to use global) here - it will get to us anyway on line 222

miripiruni · 2017-12-10T18:26:56Z

@dustyo-O linters are red

coveralls · 2017-12-19T21:12:55Z

Coverage increased (+0.002%) to 99.39% when pulling f2f24e8 on dustyo-O:requires-update into 36e253c on bem:master.

coveralls · 2017-12-22T14:49:33Z

Coverage increased (+0.002%) to 99.389% when pulling 7944c91 on dustyo-O:requires-update into e76cd62 on bem:master.

dustyo-O · 2017-12-22T14:50:27Z

fixed linters now

TODO: check that tests will fail on corrupted data (checks are checking)

dustyo-O · 2017-12-22T14:51:14Z

lib/compiler.js

-    deps.ymVarsString = deps.ymVars.map(function(item) {
-      return item + ':' + item;
-    }).join('');
+    deps.ymVars = ',' + deps.ymLibs.map(function (item) {


we make 3 maps here, can do 1 for instead, could be faster. what do you think?

@dustyo-O generate() is not runtime operation. No matter how it fast or slow we use it to generate bundles for browser.

miripiruni · 2017-12-26T12:46:30Z

test/requires-test.js

+var chaiAsPromised = require('chai-as-promised');
+chai.use(chaiAsPromised);
+
+describe('Require modules', function () {


Needed to move back tests to test/api-generate-test.js to minimize diff.

miripiruni · 2017-12-26T12:47:50Z

test/requires-test.js

+
+        vm.runInNewContext(code + EOL + bundle, sandbox);
+
+        var result = sandbox.exports.bemhtml.compile(function () {


Code style: notice that function() { in other files in the repo doesn’t have space before () brackets.

miripiruni · 2017-12-26T12:52:29Z

lib/compiler.js

-    }).join('');
+    deps.ymVars = ',' + deps.ymLibs.map(function (item) {
+      return item;
+    }).join(',');


What?

deps.ymLibs.map(function(item) { return item; }).join(',');

map is useless here.

miripiruni · 2017-12-26T12:57:39Z

lib/compiler.js

+
+        'modules.define("' + exportName + '",' +
+          JSON.stringify(deps.ymLibs) + ',' +
+          'function(provide' + deps.ymVars + ') { ' +


let’s place deps.ymVars inline?

'provide, ' + deps. … .join(', ')

miripiruni · 2017-12-26T12:58:33Z

lib/compiler.js

    ymLibs: [],
+    ymVars: '',


Why we need both ymVars and ymNames?
Previuosly, I used only ymVars: [] and ymVars.join(', ')

So many variables… I’m try to reduce their number.

miripiruni

In general we need to

minimize diff
figure out what’s changed: describe priority of module loading before and after to determine minor or major changes.

Rollback after bem/bem-xjst#493

miripiruni · 2018-02-05T12:58:00Z

JFYI: we just discussed details with @dustyo-O and decided to finish PR this evening together.

dustyo-O · 2018-02-11T12:28:37Z

ok guys, i've sawed the whole diff into step-by-step commits. i hope, this helps - happy reading!

into the f98319f i've added some alpha-version documentation, please read carefully and feel free to suggest changes. I'll add english translation before merge.

miripiruni · 2018-02-11T18:34:39Z

docs/ru/3-api.md

+
+И также, модуль из CommonJS приоритетнее модуля в YModules.
+
+Это позволяет, например, переопределять ваши модули на статические объекты в целях тестирования в соответствующих конфигурациях.


Кажется, что эту фразу можно пропустить.

miripiruni · 2018-02-11T18:37:59Z

lib/compiler.js

+      'if (typeof modules === "object") {',
+
+      // re-define YM modules to require keys
+      deps.ymNames.length !== 0 ?


Можно оставить просто deps.ymNames.length…

dustyo-O force-pushed the requires-update branch from 848b8c0 to a04ff1f Compare December 10, 2017 12:23

dustyo-O commented Dec 10, 2017

View reviewed changes

dustyo-O force-pushed the requires-update branch from a04ff1f to f2f24e8 Compare December 19, 2017 21:09

dustyo-O force-pushed the requires-update branch from f2f24e8 to 79cf722 Compare December 22, 2017 14:45

dustyo-O commented Dec 22, 2017

View reviewed changes

miripiruni reviewed Dec 26, 2017

View reviewed changes

miripiruni requested changes Dec 26, 2017

View reviewed changes

miripiruni requested review from tadatuta and qfox December 26, 2017 13:03

tadatuta added a commit to bem/bem-core that referenced this pull request Jan 22, 2018

package.json: Fix bem-xjst version

b787bc7

Rollback after bem/bem-xjst#493

tadatuta mentioned this pull request Jan 22, 2018

package.json: Fix bem-xjst version bem/bem-core#1551

Merged

tadatuta added a commit to bem/bem-core that referenced this pull request Jan 22, 2018

package.json: Fix bem-xjst version

cd3f642

Rollback after bem/bem-xjst#493

miripiruni mentioned this pull request Feb 5, 2018

Update compiler.js #492

Closed

3 tasks

miripiruni removed request for tadatuta and qfox February 5, 2018 12:56

dustyo-O force-pushed the requires-update branch 4 times, most recently from efca840 to 535b29d Compare February 11, 2018 11:07

dustyo-O added 2 commits February 11, 2018 15:01

api-generate-test: tests refactor

1c4107e

api-generate-test: new require option tests

858cecf

dustyo-O added 6 commits February 11, 2018 15:02

api: require option documentation

f98319f

compiler: YModules multiple modules require fix

086beb9

compiler: provide with YModules rather then with CommonJS

15096fb

compiler: CommonJS multiple modules require fix

a85a9c5

compiler: provide modules from different sources with Ymodules

37fba43

api-generate: modules with special symbols in names

ec667cc

dustyo-O force-pushed the requires-update branch from 535b29d to 4b8f5df Compare February 11, 2018 12:17

compiler: fixes modules require named with special symbols

3833d24

dustyo-O force-pushed the requires-update branch from 4b8f5df to 3833d24 Compare February 11, 2018 12:25

miripiruni reviewed Feb 11, 2018

View reviewed changes

fixup! api: require option documentation

7944c91

miripiruni approved these changes Feb 12, 2018

View reviewed changes

miripiruni added this to the 8.8.8 milestone Feb 12, 2018

miripiruni added the semver: patch label Feb 12, 2018

miripiruni merged commit 4b1b87c into bem:master Feb 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Module require resolving refactor #493

Module require resolving refactor #493

dustyo-O commented Dec 10, 2017 •

edited by miripiruni

Loading

dustyo-O Dec 10, 2017

miripiruni commented Dec 10, 2017

coveralls commented Dec 19, 2017 •

edited

Loading

coveralls commented Dec 22, 2017 •

edited

Loading

dustyo-O commented Dec 22, 2017

dustyo-O Dec 22, 2017

miripiruni Dec 26, 2017

miripiruni Dec 26, 2017 •

edited

Loading

miripiruni Dec 26, 2017

miripiruni Dec 26, 2017

miripiruni Dec 26, 2017

miripiruni Dec 26, 2017

miripiruni left a comment

miripiruni commented Feb 5, 2018 •

edited

Loading

dustyo-O commented Feb 11, 2018

miripiruni Feb 11, 2018

miripiruni Feb 11, 2018


		vm.runInNewContext(code + EOL + bundle, sandbox);

		var result = sandbox.exports.bemhtml.compile(function () {


		И также, модуль из CommonJS приоритетнее модуля в YModules.

		Это позволяет, например, переопределять ваши модули на статические объекты в целях тестирования в соответствующих конфигурациях.

Module require resolving refactor #493

Module require resolving refactor #493

Conversation

dustyo-O commented Dec 10, 2017 • edited by miripiruni Loading

Changes proposed in this pull request

Checklist

Choose a reason for hiding this comment

miripiruni commented Dec 10, 2017

coveralls commented Dec 19, 2017 • edited Loading

coveralls commented Dec 22, 2017 • edited Loading

dustyo-O commented Dec 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miripiruni Dec 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miripiruni left a comment

Choose a reason for hiding this comment

miripiruni commented Feb 5, 2018 • edited Loading

dustyo-O commented Feb 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustyo-O commented Dec 10, 2017 •

edited by miripiruni

Loading

coveralls commented Dec 19, 2017 •

edited

Loading

coveralls commented Dec 22, 2017 •

edited

Loading

miripiruni Dec 26, 2017 •

edited

Loading

miripiruni commented Feb 5, 2018 •

edited

Loading