Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ci(): switch the old custom build for rollup #8013

Merged
merged 24 commits into from
Jul 30, 2022
Merged

ci(): switch the old custom build for rollup #8013

merged 24 commits into from
Jul 30, 2022

Conversation

asturur
Copy link
Member

@asturur asturur commented Jun 19, 2022 •
edited

This test works, but while the code changes seems large and aren't ( all whitespace and mandatory function wrapping ),
this change itself doesn't give us much.

I made this to have a small step transition that could allow tracing back issues to particular commits, and i would like the classes to transition one at one to be able to use git commit meaningfully.

If from this point modules can be migrated one by one ( starting from leaf nodes going up to the basic one ) ok then we can merge it, otherwise is pointless.

I ll experiment more and check

To look at PR:
https://github.com/fabricjs/fabric.js/pull/8013/files?diff=unified&w=1

most of the files will have changes like this.
image

@github-actions
Copy link
Contributor

github-actions bot commented Jun 19, 2022 •
edited

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.31 |     75.7 |   85.49 |   83.01 |                                               
 fabric.js |   83.31 |     75.7 |   85.49 |   83.01 | ...,30725,30799,30810-30875,30994,31093,31329 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Jun 19, 2022 •
edited

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |    83.3 |    75.67 |   85.49 |      83 |                                               
 fabric.js |    83.3 |    75.67 |   85.49 |      83 | ...,30726,30800,30811-30876,30995,31094,31330 
-----------|---------|----------|---------|---------|-----------------------------------------------

@asturur
Copy link
Member Author

asturur commented Jun 19, 2022

ok browser import is broken

@github-actions
Copy link
Contributor

github-actions bot commented Jun 19, 2022 •
edited

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.29 |    75.66 |   85.49 |   82.99 |                                               
 fabric.js |   83.29 |    75.66 |   85.49 |   82.99 | ...,30800,30811-30876,30995,31094,31330,31489 
-----------|---------|----------|---------|---------|-----------------------------------------------

@ShaMan123
Copy link
Contributor

ShaMan123 commented Jun 19, 2022 •
edited

@asturur why don't you want to use #7839 ?
It can be used to satisfy a one by one approach
and why not use es6 exports?

@asturur
Copy link
Member Author

asturur commented Jun 19, 2022 •
edited

i m trying to add rollup without changing much else.
We can then use the transformer to change from the actual code to new code.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 19, 2022 •
edited

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |    83.3 |    75.72 |   85.48 |      83 |                                               
 fabric.js |    83.3 |    75.72 |   85.48 |      83 | ...,30791,30802-30867,30986,31085,31321,31480 
-----------|---------|----------|---------|---------|-----------------------------------------------

@asturur asturur changed the title This is a test for rollup ci(): switch the old custom build for rollup Jun 19, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jun 19, 2022 •
edited

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.32 |    75.77 |   85.54 |   83.02 |                                               
 fabric.js |   83.32 |    75.77 |   85.54 |   83.02 | ...,30790,30801-30866,30985,31084,31320,31479 
-----------|---------|----------|---------|---------|-----------------------------------------------

@asturur
Copy link
Member Author

asturur commented Jun 19, 2022

i ll make a branch from here to test partial swapping of a single file or function to a different export method. But seems to work.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 19, 2022 •
edited

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.31 |    75.74 |   85.54 |   83.01 |                                               
 fabric.js |   83.31 |    75.74 |   85.54 |   83.01 | ...,30790,30801-30866,30985,31084,31320,31479 
-----------|---------|----------|---------|---------|-----------------------------------------------

@ShaMan123
Copy link
Contributor

ShaMan123 commented Jun 20, 2022 •
edited

I have tested the new build.
My apps work with it without a problem.
Good job!

@asturur
Copy link
Member Author

asturur commented Jun 20, 2022

in a different branch i will convert a single function of the misc utils to TS, and see if that + ts + rollup-ts-whatever works
if it works it means this combination is sustainable

@ShaMan123
Copy link
Contributor

ShaMan123 commented Jun 20, 2022 •
edited

I checked the old build size with and without svg and found

file svg no svg diff
fabric.js 1039 KB 993 KB 46 KB
fabric.min.js 312 KB 296 KB 16 KB

Is it a big enough difference to support svg as an opt in or can it be built in?

@asturur
Copy link
Member Author

asturur commented Jun 20, 2022

In theory we can move all the svg related functions in a different file and developers will import them when needed.
Hopefully the big fabric.* objects goes in disuse quickly.

of course 16kb is not that big, but if you are not doing anything with svg import and export, maybe you don't want to load that code anyway.

When everything is on import/export it should be very easy to do tests.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 25, 2022 •
edited

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.36 |    75.75 |   85.54 |   82.09 |                                               
 fabric.js |   83.36 |    75.75 |   85.54 |   82.09 | ...,26629,26639-26683,26790,26877,27081,27218 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Jun 25, 2022 •
edited

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.32 |    75.72 |   85.55 |   82.04 |                                               
 fabric.js |   83.32 |    75.72 |   85.55 |   82.04 | ...,26637,26647-26691,26798,26885,27089,27226 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Jun 25, 2022 •
edited

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.33 |    75.75 |   85.55 |   82.05 |                                               
 fabric.js |   83.33 |    75.75 |   85.55 |   82.05 | ...,26637,26647-26691,26798,26885,27089,27226 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Jun 25, 2022 •
edited

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.33 |    75.75 |   85.55 |   82.05 |                                               
 fabric.js |   83.33 |    75.75 |   85.55 |   82.05 | ...,26637,26647-26691,26798,26885,27089,27226 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Jun 25, 2022 •
edited

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.33 |    75.75 |   85.55 |   82.05 |                                               
 fabric.js |   83.33 |    75.75 |   85.55 |   82.05 | ...,26637,26647-26691,26798,26885,27089,27226 
-----------|---------|----------|---------|---------|-----------------------------------------------

1 similar comment
@github-actions
Copy link
Contributor

github-actions bot commented Jun 25, 2022 •
edited

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.33 |    75.75 |   85.55 |   82.05 |                                               
 fabric.js |   83.33 |    75.75 |   85.55 |   82.05 | ...,26637,26647-26691,26798,26885,27089,27226 
-----------|---------|----------|---------|---------|-----------------------------------------------

@asturur
Copy link
Member Author

asturur commented Jun 25, 2022

ok now the PR is green, i won't touch it.
Let's see if there is some large PR that we don't want to conflict because is too complex to fix, and then we can merge this.

@ShaMan123
Copy link
Contributor

ShaMan123 commented Jun 26, 2022 •
edited

I gave a look at my open PRs and I marked 3 huge ones:

@asturur
Copy link
Member Author

asturur commented Jun 26, 2022

I re read the draggable text pr, and while i m sure is a good feature we want to add, i currently don't like how we attach it to the events, i would like to write down all the pr is doing and see if we can break the code differently. Let me look the promises pr and let's get that in, then let's scehdule some time for the draggable text after i m sure i can expose my point of view and comunicate it properly.

@stale stale bot added the stale Issue marked as stale by the stale bot label Jul 10, 2022
@ShaMan123 ShaMan123 added CI/CD and removed stale Issue marked as stale by the stale bot labels Jul 11, 2022
@fabricjs fabricjs deleted a comment from stale bot Jul 11, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jul 30, 2022 •
edited

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.15 |    74.52 |   84.49 |   80.73 |                                               
 fabric.js |   82.15 |    74.52 |   84.49 |   80.73 | ...,27373,27383-27427,27534,27621,27825,27965 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Jul 30, 2022 •
edited

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.16 |    74.55 |   84.49 |   80.74 |                                               
 fabric.js |   82.16 |    74.55 |   84.49 |   80.74 | ...,27373,27383-27427,27534,27621,27825,27965 
-----------|---------|----------|---------|---------|-----------------------------------------------

@asturur asturur merged commit 07690b6 into master Jul 30, 2022
ShaMan123
ShaMan123 reviewed Jul 30, 2022
View reviewed changes
Copy link
Contributor

@ShaMan123 ShaMan123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

test/unit/canvas.js
@@ -105,7 +105,6 @@
*/
QUnit.assert.sameImageObject = function (actual, expected) {
var a = {}, b = {};
expected = expected || REFERENCE_IMG_OBJECT;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

test/unit/group.js
@@ -889,7 +889,7 @@

group1.addRelativeToGroup(rect5);
var t = group1.calcTransformMatrix();
var pos = fabric.util.transformPoint(new fabric.Point(rect5.left, rect5.top), t);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I wanted to check something here and didn't complete

src/util/dom_style.js
@@ -35,7 +36,7 @@
var parseEl = fabric.document.createElement('div'),
supportsOpacity = typeof parseEl.style.opacity === 'string',
supportsFilters = typeof parseEl.style.filter === 'string',
reOpacity = /alpha\s*\(\s*opacity\s*=\s*([^\)]+)\)/,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you sure about this?

src/typedefs.ts
@@ -0,0 +1,11 @@
interface NominalTag<T> {
'nominalTag': T;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'nominalTag': T;
'nominalTag'?: T;

#8019 (comment)

@ShaMan123 ShaMan123 mentioned this pull request Aug 1, 2022
Closed
@asturur asturur deleted the rollup-no-ws branch September 11, 2022 23:03
ShaMan123 added a commit that referenced this pull request Dec 22, 2022
@ShaMan123
fix(): bad conflict resolution
db8739a 
d69e3dd of #8013
@ShaMan123 ShaMan123 mentioned this pull request Dec 22, 2022
Merged
frankrousseau pushed a commit to cgwire/fabric.js that referenced this pull request Jan 6, 2023
@asturur @frankrousseau
ci(): switch the old custom build for rollup (fabricjs#8013)
582d97b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShaMan123 ShaMan123 ShaMan123 left review comments

Assignees
No one assigned
Labels
CI/CD
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@asturur @ShaMan123