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.

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

cd(node): ban package.json main entry #9068

Merged
merged 3 commits into from
Dec 11, 2023
Merged

cd(node): ban package.json main entry #9068

merged 3 commits into from
Dec 11, 2023

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Jul 6, 2023

Description

Blocks the main (legacy) entry by setting it to null
This will remove all ambiguous resolution and force the dev to use fabric/node

related to #9045

Changes

Gist

In Action

@ShaMan123 ShaMan123 requested a review from asturur July 6, 2023 12:15
@github-actions
Copy link
Contributor

github-actions bot commented Jul 6, 2023

Build Stats

file / KB (diff) bundled minified
fabric 906.615 (+2.107) 304.746 (+0.757)

Copy link
Contributor Author

@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.

ready

@asturur
Copy link
Member

asturur commented Jul 9, 2023

This is acceptable since we have now 2 entry points, and definetely we want to have people using the correct entry points
Now my point is what happens if you are in jest+jsdom importing browser fabric under node?
That needs to work.

@ShaMan123
Copy link
Contributor Author

I wanted to test that but I am pretty sure using dynamic import will always work

@asturur
Copy link
Member

asturur commented Jul 19, 2023

What do you mean dynamic import? i want to merge this but i need to be sure i understand what is changing, and with this description i m not sure i get it

@asturur
Copy link
Member

asturur commented Jul 19, 2023

I also got a bug yesterday with webglprobe and i m trying to understand if it is because of this.

@ShaMan123
Copy link
Contributor Author

const fabric = await import('fabric'  or 'path/to/dist')

Copy link
Contributor Author

@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.

TODO: add import/require tests in node

@asturur
Copy link
Member

asturur commented Nov 4, 2023

Checking the npm docs, the exports and the module property we use do not exists.
They seems to be webpack stuff and not standard package.json entries.

main and browser are the only 2 supported entries?

@ShaMan123
Copy link
Contributor Author

can you point out the docs
Maybe it changed? I remember reading something about a proposal that might have not been accepted

@ShaMan123
Copy link
Contributor Author

And also we should check in MDN how browser modules work

@asturur
Copy link
Member

asturur commented Nov 4, 2023

Here is the npm doc page i was referring to:
https://docs.npmjs.com/cli/v10/configuring-npm/package-json#main

it has main and browser but doesn't mention the others.

The other one we use for sub-folders style imports i found them here:
https://webpack.js.org/guides/package-exports/

And today searching again i also found this one:
https://nodejs.org/api/packages.html#packages_exports so exports is indeed a node feature

@asturur
Copy link
Member

asturur commented Nov 4, 2023

i would like to find an example/explanations for main equal null to understand which use cases is solving

@asturur
Copy link
Member

asturur commented Nov 4, 2023

The node documentation clearly says that exports is an alternative to main.

Also found this PR to the proposal that fix the description because of a bug with the absence of a main entry when using "." export entrypoint.

https://github.com/jkrems/proposal-pkg-exports/pull/38/files

We do use "." instead of "./" and eventually we can change that, but it doesn't seem that is causing an error to us

@asturur
Copy link
Member

asturur commented Nov 24, 2023

@ShaMan123 my question is still open, why main needs to be null and not removed from the package.json ?
I can't find a list of valid values for the main entry, where did you find null as a good value?

@ShaMan123
Copy link
Contributor Author

I don't rememeber
Maybe it doesn't
I read something and opened the pr and added a test.
Try to remove and see what happens to test

Copy link
Contributor

Coverage after merging cd-no-main-entry into master will be

82.76%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
index.node.ts7.69%100%0%14.29%17, 20, 23, 35, 38, 41
src
   ClassRegistry.ts100%100%100%100%
   Collection.ts94.85%94.83%87.10%97.14%109, 112, 215–216, 241–242
   CommonMethods.ts96.55%87.50%100%100%9
   Intersection.ts100%100%100%100%
   Observable.ts87.76%85.29%87.50%89.58%134–135, 160–161, 32–33, 41, 50, 78, 89
   Point.ts100%100%100%100%
   Shadow.ts98.36%95.65%100%100%178
   cache.ts97.06%90%100%100%57
   config.ts75%66.67%66.67%82.76%130, 138, 138, 138, 138, 138–140, 151–153
   constants.ts100%100%100%100%
src/LayoutManager
   LayoutManager.ts85.80%90.91%68%87.32%127–128, 130–131, 131, 131, 131, 131, 231, 294–295, 299, 39, 60, 78
   constants.ts100%100%100%100%
src/LayoutManager/LayoutStrategies
   ClipPathLayout.ts81.58%72.22%100%88.24%29–30, 40, 52, 55–56, 63
   FitContentLayout.ts100%100%100%100%
   FixedLayout.ts11.11%0%0%25%19–20, 22, 22, 22, 22, 22
   LayoutStrategy.ts86.54%64.71%100%96.30%36, 43, 43, 43, 51, 78–79
   utils.ts89.47%80%100%100%28, 34
src/Pattern
   Pattern.ts92.31%91.89%90%93.55%119, 130, 139, 32, 95
src/brushes
   BaseBrush.ts100%100%100%100%
   CircleBrush.ts0%0%0%0%108, 108, 108, 110, 112, 114–116, 118–121, 128–129, 13, 136, 138, 23–24, 32–36, 40–44, 51–54, 62–66, 68, 76, 76, 76, 76, 76–77, 79, 79, 79–82, 84, 92–93, 95, 97–99
   PatternBrush.ts97.06%87.50%100%100%21
   PencilBrush.ts91.06%82.35%100%93.81%122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
   SprayBrush.ts0%0%0%0%107, 107, 107, 107, 107–108, 110–111, 118–119, 121, 123–127, 136, 140–141, 141, 148, 148, 148–151, 153–156, 160–161, 163, 165–168, 17, 171, 178–179, 18, 181, 183–184, 186, 193–194, 196–197, 20, 200, 200, 207, 207, 21, 211, 22, 22, 22–24, 28, 32, 39, 46, 53, 60, 67, 84–86, 94–96, 98–99
src/canvas
   Canvas.ts79%76.99%83.05%79.84%1002–1003, 1003, 1003–1004, 1010, 1012, 1040–1042, 1045–1046, 1050–1051, 1174–1176, 1179–1180, 1253, 1368, 1490, 155, 180, 286, 286–291, 296, 319–320, 325–330, 350, 350, 350–351, 351, 351–352, 360, 365–366, 366, 366–367, 369, 378, 384–385, 385, 385, 39, 428, 43, 436, 440, 440, 440–441, 443, 525–526, 526, 526–527, 533, 533, 533–535, 555, 557, 557, 557–558, 558, 558, 561, 561, 561–562, 565, 574–575, 577–578, 580, 580–581, 583–584, 596–597, 597, 597–598, 600–605, 611, 618, 655, 655, 655, 657, 659–664, 670, 676, 676, 676–677, 679, 682, 687, 700, 728, 780–781, 781, 781, 781, 781, 781, 784–785, 788, 788–790, 793–794, 876, 883, 883, 883, 896, 929, 950–951, 967–968, 968, 968–970, 973–974, 974, 974, 976, 984, 984, 984–986, 986, 986, 993–994
   CanvasOptions.ts100%100%100%100%
   SelectableCanvas.ts93.21%91.60%94.44%94.22%1013, 1021, 1140, 1142, 1144–1145, 302, 472–473, 475–476, 476, 476, 525–526, 587–588, 601, 641–643, 675, 680–681, 708–709, 769–770, 775–779, 781, 940, 940–941, 944, 964, 964
   StaticCanvas.ts96.78%93.09%100%98.53%1019, 1029, 1081–1082, 1085, 1120–1121, 1197, 1206, 1206, 1210, 1210, 1257–1258, 178–179, 195, 559, 571–572, 902–903, 903, 903–904
   StaticCanvasOptions.ts100%100%100%100%
   TextEditingManager.ts86%71.43%91.67%91.67%17, 17, 17–18, 18, 18
src/canvas/DOMManagers
   CanvasDOMManager.ts95.52%70%100%100%21–22, 29
   StaticCanvasDOMManager.ts97.50%88.89%100%100%33
   util.ts86.67%80.56%83.33%93.94%14, 26, 63–64, 67, 67, 74, 93–94
src/color
   Color.ts94.96%91.67%96.30%96.05%233, 258–259, 267–268, 48
   color_map.ts100%100%100%100%
   constants.ts100%100%100%100%
   util.ts85.71%76.92%100%89.74%55–56, 56, 58, 58, 58–59, 61–62, 89
src/controls
   Control.ts94.44%93.10%91.67%96.77%183, 249, 354
   changeWidth.ts100%100%100%100%
   commonControls.ts100%100%100%100%
   controlRendering.ts81.63%78%100%84.78%106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
   drag.ts100%100%100%100%
   fireEvent.ts88.89%75%100%100%13
   polyControl.ts8.57%0%0%15.79%100, 106, 111, 126, 126, 126, 126, 126, 128–131, 131, 134, 141, 20, 28–32, 32, 32, 32, 32, 32, 32, 32, 53–59, 59, 59, 59, 59, 61, 66–67, 69, 79, 85–87, 87, 89, 92–93, 93, 93, 93, 93, 95
   rotate.ts19.57%12.50%50%21.43%41,

@asturur asturur merged commit e4ac8b5 into master Dec 11, 2023
24 checks passed
@asturur asturur deleted the cd-no-main-entry branch December 11, 2023 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants