Use separate module system to properly handle errors #111

zamotany · 2017-09-15T14:10:40Z

Few notes

Related to #81

Things done in this PR

Custom module system with separate cache
- Full control over transpilation, no need for babel-register
- No babel-register leakage to main module system
Proper errors from source maps
Moved some tests to __integration-tests__

Things to do

mock native module module in module system 😛
unit tests for preval-extract modules
unit tests for babel/lib modules
more options for transpilation exposed in linaria preset (in follow-up PR)
integration test optimization (in follow-up PR)
calculate column name for styles correctly (in follow-up PR)
squash commits

codecov · 2017-09-15T17:08:18Z

Codecov Report

Merging #111 into master will increase coverage by 0.77%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #111      +/-   ##
==========================================
+ Coverage   96.87%   97.65%   +0.77%     
==========================================
  Files          15       18       +3     
  Lines         256      341      +85     
  Branches       49       59      +10     
==========================================
+ Hits          248      333      +85     
  Misses          8        8

Impacted Files	Coverage Δ
src/babel/preval-extract/resolveSource.js	`100% <100%> (ø)`	⬆️
src/babel/preval-extract/getReplacement.js	`100% <100%> (ø)`
src/babel/preval-extract/prevalStyles.js	`100% <100%> (ø)`
src/babel/lib/errorUtils.js	`100% <100%> (ø)`
src/babel/preval-extract/index.js	`100% <100%> (ø)`	⬆️
src/babel/preval-extract/validators.js	`100% <100%> (ø)`	⬆️
src/babel/preval-extract/extractStyles.js	`100% <100%> (ø)`	⬆️
src/babel/lib/moduleSystem.js	`100% <100%> (ø)`
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08b6425...09dd8f0. Read the comment docs.

satya164 · 2017-09-16T09:49:24Z

src/babel/__integration-tests__/__snapshots__/preval-extract.spec.js.snap

+> 4 | width: \${document.width};
+    |          ^
+  5 | \`;
+    at File.buildCodeFrameError (<<REPLACED>>/node_modules/babel-core/lib/transformation/file/index.js:427:15)


This stacktrace seems confusing

This stacktrace is negligible here. Could not find a reference to error is thrown even before evaluating, also this error is from previous PR.

satya164 · 2017-09-17T20:41:13Z

src/babel/__integration-tests__/__utils__/exec.js

+  pluginOptions: any = { cache: false, extract: false },
+  babelOptions: any = { filename: 'test.js' }
+) {
+  const PATH_TO_TRANSPILE_BIN = path.join(__dirname, './transpile.js');


nit: don't need the ./

satya164 · 2017-09-17T20:43:27Z

src/babel/__integration-tests__/__utils__/extract.js

+const filenames = [];
+
+fs.writeFileSync = (filename, data, opts) => {
+  if (/\.css$/.test(filename)) {


A comment will be nice to tell what's going on here

thymikee

Left some small comments, but generally looks good. Now it makes sense why we need our own module mock for prevaled stuff, stellar work 👍

thymikee · 2017-09-20T18:21:58Z

src/babel/__integration-tests__/preval-extract.spec.js

+expect.addSnapshotSerializer({
+  test: val => val && val.toString && val.toString().includes(process.cwd()),
+  serialize,
+  print: serialize,


you should use either serialize or print, both are not necessary.

thymikee · 2017-09-20T18:38:17Z

src/babel/__integration-tests__/preval-extract.spec.js

+      font-size: 3em;
+    \`;
+    `);
+    expect(code.includes('font-size: 3em;')).toBeTruthy();


You can use toMatch, also snapshot is usually not needed when you're matching strings (you're already testing the transpiled output in other places).

expect(code).toMatch('font-size: 3em;')

thymikee · 2017-09-20T18:40:22Z

src/babel/__integration-tests__/preval-extract.spec.js

+      `);
+    } catch (error) {
+      expect(stripAnsi(error.toString())).toMatch(/> 10 \| {5}throw new Error/);
+      expect(error).toMatchSnapshot();


Either snapshot or toMatch.

why not both?

Because it's just redundant

thymikee · 2017-09-20T18:45:14Z

src/babel/__integration-tests__/preval-extract.spec.js

+      codeWithSlugFromFilename
+    );
+
+    expect(classnameWithSlugFromContent).not.toBeNull();


RegExp.test is faster than exec and returns a boolean, also global flag is not needed.

expect(/header = '(header__[a-z0-9]+)'/.test(codeWithSlugFromContent)).toBe(true)

But I see you're already using exec everywhere, so I can live with that.

I need to use exec, test is insufficient for the test.

thymikee · 2017-09-20T18:51:27Z

src/babel/__integration-tests__/preval-extract.spec.js

+    expect(css).toMatchSnapshot();
+  });
+
+  it('should preval const and let without transpilation to var', () => {


Are you actually testing that here?

thymikee · 2017-09-20T19:05:51Z

src/babel/__integration-tests__/preval-extract.spec.js

+      expect(data).toMatchSnapshot();
+    });
+  });
+});


Generally in this whole file, when you have toMatch next to toMatchSnapshot – pick one, in most cases I'd pick toMatch because it's simple and clear enough and doesn't force me to jump between files

thymikee · 2017-09-20T19:07:14Z

src/babel/lib/__tests__/__snapshots__/errorUtils.spec.js.snap

+"Error: Test message
+[0m [90m 1 | [39m[90m// comment[39m
+[31m[1m>[22m[39m[90m 2 | [39m[36mthrow[39m [36mnew[39m [33mError[39m([32m\\"test\\"[39m)[33m;[39m
+ [90m   | [39m       [31m[1m^[22m[39m[0m


Can we strip these colors? I can live without them being here and it's pretty hard to read in this form.

thymikee · 2017-09-20T19:57:10Z

src/babel/lib/moduleSystem.js

+    const filename = resolveMock(moduleId);
+
+    // Native Node modules
+    if (filename === moduleId && !moduleId.startsWith('/')) {


How about windows?

Suggest the solution

thymikee · 2017-09-20T19:57:39Z

src/babel/lib/__tests__/moduleSystem.spec.js

+    expect(ModuleMock('file.js')).toEqual(new Module('file.js'));
+    const cache = ModuleMock._cache;
+    expect(ModuleMock._debug()).toBeUndefined();
+    expect(ModuleMock._preloadModules()).toBeUndefined();


Things like this should be handled by Flow, just sayin'

thymikee · 2017-09-20T19:58:51Z

src/babel/lib/moduleSystem.js

+ * ==============================================
+ * To avoid leakage from evaled code to module cache in current context,
+ * for example with `babel-register` we provide our custom module system.
+ * It designed to mimic the native node one, with the exception being


thymikee · 2017-09-21T09:27:50Z

Rebase to master and I think we're good to go. @satya164 ?

satya164 · 2017-09-21T09:33:03Z

src/babel/__integration-tests__/preval-extract.spec.js

+    const { code } = transpile(dedent`
+    /* linaria-preval */
+    const header = css\`
+      font-size: {2 + 1}em;


did you mean ${2 + 1}? this syntax seems wrong?

satya164 · 2017-09-21T09:49:24Z

src/babel/lib/moduleSystem.js

+}
+
+export function clearModulesCache() {
+  Object.keys(modulesCache).forEach(moduleId => {


won't it be better just to do modulesCache = {}?

Will break references. Besides it's used only in tests.

satya164 · 2017-09-21T12:32:13Z

Is it ready? There seem to be some small comments which need to be addressed

ARE YOU READY?

zamotany added the status: in progress 🚧 Someone is working on the issue label Sep 15, 2017

satya164 requested changes Sep 18, 2017

View reviewed changes

zamotany changed the title ~~Use separate module system and properly handle errors~~ Use separate module system to properly handle errors Sep 19, 2017

callstack deleted a comment from satya164 Sep 19, 2017

use custom module system to generate proper stacktrace

057984a

zamotany force-pushed the preval-code-frame branch from 3d8b4e6 to 057984a Compare September 20, 2017 09:10

zamotany removed the status: in progress 🚧 Someone is working on the issue label Sep 20, 2017

thymikee reviewed Sep 20, 2017

View reviewed changes

fixes and tweaks

5b9aee5

zamotany added 2 commits September 21, 2017 11:32

use snapshots only to check errors

8420c05

Merge branch 'master' into preval-code-frame

6fe0587

satya164 requested changes Sep 21, 2017

View reviewed changes

tweak test

09dd8f0

satya164 approved these changes Sep 21, 2017

View reviewed changes

thymikee approved these changes Sep 21, 2017

View reviewed changes

satya164 merged commit 1c61aa4 into master Sep 21, 2017

satya164 deleted the preval-code-frame branch September 21, 2017 12:43

satya164 mentioned this pull request Sep 21, 2017

Ensure we throw nice errors when users do something unsupported #52

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use separate module system to properly handle errors #111

Use separate module system to properly handle errors #111

zamotany commented Sep 15, 2017 •

edited

codecov bot commented Sep 15, 2017 •

edited

satya164 Sep 16, 2017

zamotany Sep 18, 2017

satya164 Sep 17, 2017

satya164 Sep 17, 2017

thymikee left a comment

thymikee Sep 20, 2017

thymikee Sep 20, 2017

thymikee Sep 20, 2017

zamotany Sep 21, 2017

thymikee Sep 21, 2017

thymikee Sep 20, 2017

zamotany Sep 21, 2017

thymikee Sep 20, 2017

thymikee Sep 20, 2017

thymikee Sep 20, 2017

thymikee Sep 20, 2017

zamotany Sep 21, 2017

thymikee Sep 20, 2017 •

edited

thymikee Sep 20, 2017

thymikee commented Sep 21, 2017

satya164 Sep 21, 2017

satya164 Sep 21, 2017

zamotany Sep 21, 2017

satya164 commented Sep 21, 2017 •

edited

Use separate module system to properly handle errors #111

Use separate module system to properly handle errors #111

Conversation

zamotany commented Sep 15, 2017 • edited

Few notes

Things done in this PR

Things to do

codecov bot commented Sep 15, 2017 • edited

Codecov Report

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

thymikee left a 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

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

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

thymikee Sep 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thymikee commented Sep 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satya164 commented Sep 21, 2017 • edited

zamotany commented Sep 15, 2017 •

edited

codecov bot commented Sep 15, 2017 •

edited

thymikee Sep 20, 2017 •

edited

satya164 commented Sep 21, 2017 •

edited