new styles extractor by Anber · Pull Request #353 · callstack/linaria

Anber · 2019-03-12T22:08:36Z

Summary

This is a PoC of more advanced styles extractor. It can evaluate almost any js code whereas current extractor has problems with code like this:

const object = { fontSize: 12 };
object.fontWeight = 'bold';
export default styled.h1`
  ${object};
`;

Unlike current implementation, this extractor does not evaluate expressions immediately in TaggedTemplateExpression visitor but collects them for later evaluation. Then it builds a dependency graph for all expressions and strips all unrelated code from a module. On the last stage, it does a batch evaluation of all expressions.

I've created this PR in WIP stage because I had a bit painful merge after 1.3.1 release and I also found your typescript branch :)
So if you like it I will do some refactoring and rewrite to TS.

Test plan

New tests were added, old tests still work except one test with invalid code and two tests that check dependency extraction during evaluation.

satya164 · 2019-03-12T22:13:31Z

So awesome! I didn't have time to look at the code yet but will be looking when I have time.

Don't worry about my TypeScript branch for now. I don't think I'll have time to complete it this month. But if you want to use TypeScript, maybe you can use it for the new code you write. Then when we migrate, we will only need to migrate the old code.

Anber · 2019-03-12T22:27:12Z

It would be better if you didn't look at the code now :)
I will have finished it and cover with comments by the end of the week.

Anber · 2019-03-14T20:48:55Z

I guess it's ready for review.
There are still two failed tests. I will try to find the reason of failure but it would be great if you checked the tests itself.

zamotany · 2019-03-15T12:05:21Z

Hey, @Anber thank you for your PR. We really appreciate your contribution and we would like to have it merged, but at the time being I'm having a hard time understanding the code without types, so here's what we are going to do: I'll finish up the migration to TS and then I'll ask you to migrate your code. Sorry for requesting more work from your side. I hope you'll understand our decision.

Anber · 2019-03-15T12:11:26Z

Hi!

It’s ok. Can I take @satya164’s branch as a base for migration?

zamotany · 2019-03-15T12:12:34Z

@Anber Yes, I'll be finishing up the migration on @satya164's branch.

Anber · 2019-03-15T19:45:43Z

I've finished with shaker.ts, depsGraph.ts and scope.ts. The rest I'll migrate when you finish with the base branch.

Anber · 2019-03-26T11:36:50Z

@zamotany I have some free time, can I help you with migration?

zamotany · 2019-03-27T13:30:11Z

@Anber I'm in the process of setting up the infra for the migration (build pipeline, ESLint, etc), once it's done, you can jump in and help me migrate files. I was thinking about migrating the source code only first (excluding test, fixtures, website etc), since it can be done later.

Anber · 2019-03-27T13:32:39Z

@zamotany Ok. Let's me know when you will be ready!

zamotany · 2019-04-02T10:51:18Z

@Anber I have setup the infra for the migration here: https://github.com/callstack/linaria/tree/feature/typescript-migration so you can do PRs agains this branch feature/typescript-migration. The script typecheck and lint might fail, but the important one is test - the test must always pass. I'll be working on making typecheck and lint scripts to pass with both JS (w/ Flow) and TS present.

Anber · 2019-04-02T14:14:02Z

I've changed the PR base. There are still two failed tests with dependencies that I mentioned before but everything else seems to be ok.
I'll finish with typings when typecheck is ready.

Anber · 2019-04-02T14:32:33Z

@satya164 could you please check tests handles wrapping another styled component and handles escapes properly?

handles complex component expects core-js/modules/es6.string.raw as a dependency and it looks strange to me;
handles wrapping another styled component expects ../react but I think, we don't really need it for extraction, so it's not a real dependency.

Anber · 2019-04-03T17:35:18Z

I've migrated src/babel to TS.

Anber · 2019-04-04T13:04:14Z

➜ linaria ✗ yarn tsc --noEmit
✨ Done in 7.13s.

zamotany · 2019-04-04T16:38:07Z

@Anber Awesome, incredible work, do you want to create a separate PR with the TS migration for src/babel or do you want to keep it in this one.
Maybe we should create a separate branch like develop from master for TS and avoid blocking master for critical bug fixes. WDYT? @Anber @satya164 @thymikee

Anber · 2019-04-04T17:46:14Z

New branch for the next big release sounds good. We can collect all potential BC features in it and publish something like canary.

thymikee · 2019-04-05T08:02:50Z

+1 for migrating under a dedicated branch

zamotany · 2019-04-05T10:30:13Z

@Anber I have created a dedicated branch develop (https://github.com/callstack/linaria/tree/develop) from master, so can you open a PR agains that branch with just old files (excluding the new styles extractor) migrated to TS? If there are some JS files left, don't worry about those.

Anber · 2019-04-05T10:34:20Z

I'll try to separate extractor and migration to two different PR but it wouldn't be simple, so I need some time.

zamotany · 2019-04-08T10:38:47Z

@Anber just extract the files that are common to develop and extractor branch and make a PR with those. I'll migrate the missing ones. The reason I'm not taking your files that are common and integrating them into develop, instead I ask you to do so, it that I don't want to take away your contributions from the history.

Also, how about moving the files related to dep grath etc to a separate directory? Instead them living in utils it would be sth like dependencyExtractor??

Anber · 2019-04-08T10:52:55Z

Hi @zamotany!
I'll do it in the next two or three days.

how about moving the files related to dep grath etc to a separate directory

It's a good idea!
depsGraph is closely linked to shaker and evaluate, so maybe we will move all of them to the separate directory evaluate or smth like this?

zamotany · 2019-04-08T10:57:57Z

@Anber that's fine too. Also I think you can update those 2 failing snapshots.

zamotany · 2019-04-11T11:45:43Z

@Anber can you resolve the conflicts?

callstack-bot · 2019-04-11T12:11:13Z

Hey @Anber, thank you for your pull request 🤗.
The coverage report for this branch can be viewed here.

Anber · 2019-04-11T12:13:23Z

@zamotany, I hope, now it's ready for review :)

zamotany

Maybe we should call dumpNode if some environment variable is set? Like LINARIA_DEBUG?

zamotany · 2019-04-11T15:27:26Z

+    return (
+      this.edges
+        // shallowEqual is not defined
+        .filter(([a]) => (t as any).shallowEqual(a, like))


Can you explain what this comment is about and why are we casting t to any??

Oops, I've missed the second part of the comment in babel typings. Anyway, I'll add this shallowEqual to DefinitelyTyped and remove this any when the new version of @types/babel__core is published.

zamotany · 2019-04-11T15:27:49Z

+    return this.bindingsDependents.get(id) || [];
+  }
+
+  findDependencies(like: object) {


Suggested change

findDependencies(like: object) {

findDependencies(like: Object) {

@zamotany I think object is preferred

zamotany · 2019-04-11T15:28:01Z

+    );
+  }
+
+  findDependents(like: object) {


Suggested change

findDependents(like: object) {

findDependents(like: Object) {

zamotany · 2019-04-11T15:29:00Z

+  }
+}
+
+class GraphBuilder {


Can we split those 2 classes into separate files?

zamotany · 2019-04-11T15:30:52Z

+    }
+  }
+
+  private readonly graph: Graph;


Suggested change

private readonly graph: Graph;

private readonly scope: ScopeManager = new ScopeManager();

zamotany · 2019-04-11T15:44:53Z

+
+  const { type } = node;
+  // VISITOR_KEYS is not defined in babel typings
+  const keys: [keyof T] = (t as any).VISITOR_KEYS[type] || [];


Maybe let's keep VISITOR_KEYS in some file, types.ts maybe, since it's used in multiple files.

zamotany · 2019-04-11T15:45:49Z

+    }
+  }
+
+  return Object.keys(changes).length ? Object.assign({}, node, changes) : node;


Suggested change

return Object.keys(changes).length ? Object.assign({}, node, changes) : node;

return Object.keys(changes).length

? { ...node, ...changes }

: node;

zamotany · 2019-04-11T15:46:32Z

+ * Adds to the end of module export of array of evaluated values or evaluation errors.
+ * Returns new AST and an array of external dependencies.
+ */
+function shake(


Suggested change

function shake(

export default function shake(

zamotany · 2019-04-11T15:46:40Z

+  ];
+}
+
+export default shake;


Suggested change

export default shake;

zamotany · 2019-04-11T15:47:43Z

+  }
+
+  const shaken = shakeNode(rootPath, alive) as t.Program;
+  // dumpNode(program, alive);


Suggested change

// dumpNode(program, alive);

Anber · 2019-04-12T19:05:52Z

@zamotany, done. I've split depsGraph into 4 files.

zamotany

Looks awesome but there is a problem - the website is failing to build:

zamotany · 2019-04-16T14:26:26Z

it's failing on this forEach (Visitors.ts:250:16):

const declared = this.meta.get('declared') as t.Identifier[];
      declared.forEach(n =>
        this.graph.externalDeps.push({
          source,
          local: n,
          imported: null,
        })
      );

Anber · 2019-04-16T14:49:34Z

It had expected some variable declaration, but received bare require(…).

zamotany · 2019-04-17T14:39:37Z

Huge thanks @Anber 👍
I'll finish the migration of other JS files and hopefully soon, we'll do a release.

Anber · 2019-04-17T18:36:34Z

Thank you!
Let me know if I can help you.

Anber · 2019-05-28T12:55:07Z

handles complex component expects core-js/modules/es6.string.raw as a dependency and it looks strange to me;

It was an error :) I'll fix it soon.

Anber force-pushed the extractor branch from 85df61c to e56f454 Compare March 14, 2019 20:39

Anber changed the title ~~wip: new styles extractor~~ new styles extractor Mar 15, 2019

satya164 requested a review from zamotany March 15, 2019 09:21

Anber force-pushed the extractor branch from 36e90d7 to ae86bae Compare March 15, 2019 19:41

Anber changed the base branch from master to @satya164/typescript March 15, 2019 19:41

Anber force-pushed the extractor branch from ae86bae to a3bba74 Compare April 2, 2019 14:04

Anber changed the base branch from @satya164/typescript to feature/typescript-migration April 2, 2019 14:04

Anber force-pushed the extractor branch from a4a1f8b to 3f0431b Compare April 4, 2019 13:28

Anber changed the base branch from feature/typescript-migration to develop April 5, 2019 10:31

Anber mentioned this pull request Apr 10, 2019

Migrate to TypeScript #386

Merged

feat: new styles extractor

422a806

Anber force-pushed the extractor branch from 3f0431b to 422a806 Compare April 11, 2019 12:10

zamotany suggested changes Apr 11, 2019

View reviewed changes

WIP: code review

993ae10

zamotany suggested changes Apr 16, 2019

View reviewed changes

WIP: fix website build

65838f4

zamotany approved these changes Apr 17, 2019

View reviewed changes

zamotany merged commit d57e859 into callstack:develop Apr 17, 2019

Anber added a commit that referenced this pull request Dec 26, 2019

feat: new styles extractor (#353)

3fb8024

aweber1 mentioned this pull request Jan 8, 2020

1.4.0-beta.0 doesn't have some features and files from 1.4.0-alpha.1 #523

Closed

Anber added a commit that referenced this pull request Jan 12, 2020

feat: new styles extractor (#353)

ffd3b14

Anber added a commit that referenced this pull request Jan 14, 2020

feat: new styles extractor (#353)

4f5fc97

Anber added a commit that referenced this pull request Feb 4, 2020

feat: new styles extractor (#353)

be48299

Anber added a commit that referenced this pull request Feb 9, 2020

feat: new styles extractor (#353)

219f59d

Anber added a commit that referenced this pull request Feb 10, 2020

feat: new styles extractor (#353)

49d5a66

Anber mentioned this pull request Feb 12, 2020

[1.4.0-beta.0/1/2] - Build Error - Using generated classnames as selectors fails #540

Closed

	findDependencies(like: object) {
	findDependencies(like: Object) {

	findDependents(like: object) {
	findDependents(like: Object) {

	private readonly graph: Graph;
	private readonly scope: ScopeManager = new ScopeManager();

Uh oh!

Conversation

Anber commented Mar 12, 2019

Uh oh!

satya164 commented Mar 12, 2019

Uh oh!

Anber commented Mar 12, 2019

Uh oh!

Anber commented Mar 14, 2019

Uh oh!

zamotany commented Mar 15, 2019

Uh oh!

Anber commented Mar 15, 2019

Uh oh!

zamotany commented Mar 15, 2019

Uh oh!

Anber commented Mar 15, 2019

Uh oh!

Anber commented Mar 26, 2019

Uh oh!

zamotany commented Mar 27, 2019

Uh oh!

Anber commented Mar 27, 2019

Uh oh!

zamotany commented Apr 2, 2019

Uh oh!

Anber commented Apr 2, 2019

Uh oh!

Anber commented Apr 2, 2019

Uh oh!

Anber commented Apr 3, 2019

Uh oh!

Anber commented Apr 4, 2019

Uh oh!

zamotany commented Apr 4, 2019

Uh oh!

Anber commented Apr 4, 2019

Uh oh!

thymikee commented Apr 5, 2019

Uh oh!

zamotany commented Apr 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Anber commented Apr 5, 2019

Uh oh!

zamotany commented Apr 8, 2019

Uh oh!

Anber commented Apr 8, 2019

Uh oh!

zamotany commented Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zamotany commented Apr 11, 2019

Uh oh!

callstack-bot commented Apr 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Anber commented Apr 11, 2019

Uh oh!

zamotany left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zamotany commented Apr 5, 2019 •

edited

Loading

zamotany commented Apr 8, 2019 •

edited

Loading

callstack-bot commented Apr 11, 2019 •

edited

Loading

zamotany commented Apr 16, 2019 •

edited

Loading