[WIP] rollup and ESM export #1858

LoicMahieu · 2018-11-06T11:46:21Z

1. Summary

It's a quite big PR for my first contribution. I'm sorry, I would have preferred to find something easier to do at first. I tried something simple, it's in the garage... I could not develop it on top of the actual build system due to performance.

There is some problem in the current build implementation:

All modules (except netlify-cms) produces a UMD build which, I think, is never used by anyone. (Maybe there is a case for import manually a widget or something else ?)
netlify-cms module have an "hacky" import. Since we don't want to bundle once again a UMD bundle from other UMD bundles, it imports directly /src.
netlify-cms have a bunch of dependencies that are not used in production, since every dependencies are bundled.
Build is slow. In my machine, I have waited more than 5 minutes for a complete build with all CPU to the top!

Important changes:

Switch to rollup instead of webpack for all modules (except netlify-cms). It only produces a ESM build.
netlify-cms still build an UMD package from the ESM with webpack.
icons: switch to rollup-plugin-react-svg for SVG to React transformation. It does not seem to change much.
Remove alias and module-resolver. It was initially proposed in PR netlify-cms-core: remove alias and module-resolver #1847. I did that because I found difficulties to implement it with Rollup and...... since I don't like it much... 😚 But it should be possible to use it. If you want to see them come back, yell at me and I'll do what's necessary.

Advantages:

Build time is improved! On my machine, 62.84s for a full build.
Well described by @GabeMedrash in:
- Refactor library build process to output CommonJS build, ES module build, and a bundle #1829
- WIP: fix: transpile bundle to ES5 #1770 (comment)

Fixes issues:

Breaking changes

All modules (except netlify-cms) will not exports a main file but only an module file.
It does not introduce any breaking change on netlify-cms.
I don't know if they are used separately/directly.
I don't see any other breaking change 😅

2. Test plan

CI should confirm that build works. Tested on my machine, build and develop works well!

3. A picture of a cute animal

As big as this PR!

Some advantages to remove aliases: - Better integration with code editors (VSCode in mind) - Easier to move to another bundler

verythorough · 2018-11-06T11:50:49Z

Deploy preview for netlify-cms-www ready!

Built with commit 9292cb5

https://deploy-preview-1858--netlify-cms-www.netlify.com

verythorough · 2018-11-06T11:52:22Z

Deploy preview for cms-demo ready!

Built with commit 9292cb5

https://deploy-preview-1858--cms-demo.netlify.com

LoicMahieu · 2018-11-06T11:53:02Z

Current problems:

Rollup watch have an issue (I think rollup fails to resolve entry file when using --watch, seemingly at random rollup/rollup#982).
When I open a random file, just hit cmd+s and it produces this kind of error:
no such file or directory, open 'react'.
(It is not always react)
Keep investigating....
netlify-cms: show this warning: "export 'BitBucketBackend' was not found in 'netlify-cms-backend-bitbucket'
I don't know yet why this warning is there but BitBucketBackend is well exported!

My first wish at this time is to receive your opinion about this implementation and knowing if I could go further.
I want to invest my time and that of my team on this fantastic project!

Thanks for your attention!

erquhart · 2018-11-06T18:17:38Z

First of all, thank you! This is awesome work in a critical area of need for the project!! 👏👏

Whenever a massive changeset comes through, it's good to slim down where we can. I'd really like to see removal of module-resolver dropped from this PR, as it adds a lot of noise. Hopefully we can get rollup to play nice with it.

Checking it out locally now.

This reverts commit 376ded3. # Conflicts: # packages/netlify-cms-core/babel.config.js # packages/netlify-cms-core/src/reducers/auth.js

For an unkonwn reason, some aliased import does not work in Rollup like Webpack. For example, `import ‘Redux’` results in `require(‘./redux/index.js’)` in the ESM bundle and it is not bundled.

LoicMahieu · 2018-11-07T12:17:54Z

Hi @erquhart
Thank you for your interest!

I agree that removal of module-resolver adds a lot of noise. Unfortunately, this plugin does not deal well with rollup.
I reverted the commit (I'll rebase this branch later) and I had to modify each "implicit index.js import" for an "explicitly import", exemple:

- import { getAsset } from 'Reducers';
+ import { getAsset } from 'Reducers/index';

Otherwise, rollup output contains:

import { getAsset } from "./redux/index";

In that case, Rollup does not find the module and left the import into the bundle...
Since import explicitly /index is really ugly, I don't think this branch should be merged as is.

I tried to solves aliases from Rollup with plugin:

rollup-plugin-alias : seems to have also a problem with "implicit index.js".

I need to investigate more about it but... I am convicted that aliases are not necessary 😬

erquhart · 2018-11-07T15:22:20Z

I've restructured this entire codebase twice in the last twelve months; relative imports were a ridiculous pain to deal with. Let's see if we can find a way to make aliases work.

erquhart · 2018-11-07T15:24:43Z

This comment by Rich Harris is right on - adding /index is fine, let's just do that.

Benaiah · 2018-11-07T17:22:33Z

I'm also fine with just importing using index; it's not much more verbose and makes it a bit clearer for people who aren't already aware that index.js is the default entry point for a folder.

LoicMahieu · 2018-11-08T09:04:27Z

Yep okay let's keep the /index.
I tried to use rollup-plugin-alias but it does not support root as babel-plugin-module-resolver. So paths like UI, UI/FooComponent, EditorWidgets etc... does not work.
Anyway, it works well with module-resolver.

LoicMahieu · 2018-11-09T16:51:23Z

I remove the WIP. In my use cases, it works well. Need your feedback ;) Thanks!

erquhart · 2018-11-12T16:35:34Z

Off today but I'll dig into this tomorrow 👍

erquhart

Just a couple of small changes (plus merge conflict resolution) and this is good to merge - awesome work @LoicMahieu, this is a clutch PR!! 👏👏

erquhart · 2018-11-22T03:13:35Z

packages/netlify-cms/webpack.config.js

-const pkg = require('./package.json');
-const { plugins } = require('../../scripts/webpack');
-const coreWebpackConfig = require('../netlify-cms-core/webpack.config.js');
+const pkg = require(path.join(process.cwd(), 'package.json'));


I'd opt for __dirname instead of process.cwd() here, in case the cli is run from somewhere other than root.

pkg here should be the specific package, not the root. The logic does not change from the current version.

packages/netlify-cms-core/package.json

erquhart · 2018-11-22T03:37:59Z

Update: looks like our UMD build went from ~2MB to ~4MB. We'll have to find a way to address that before merging.

alexandernanberg · 2018-11-23T15:05:53Z

I've restructured this entire codebase twice in the last twelve months; relative imports were a ridiculous pain to deal with. Let's see if we can find a way to make aliases work.

Just curious what was a pain with relative paths? In my experience it's the other way around 😛

erquhart · 2018-11-23T20:40:23Z

When you move a file to a different level of nesting, all relative imports break. When you move 80 files to a different level of nesting, your sanity breaks.

_{Sent with GitHawk}

alexandernanberg · 2018-11-26T14:10:25Z

@LoicMahieu Do you have enough time to get this through to the finish line? If not I could have a look and try to fix the remaining stuff 😄

erquhart · 2018-11-26T23:36:42Z

GitHub needs a 💪reaction. You're killing it @alexandernanberg.

LoicMahieu · 2018-11-29T12:25:29Z

Hi @alexandernanberg
Any help is welcome 😉 I fixed the review above. Indeed I don't have many time to devote 😣
The problems that are still there:

UMD bundle size (from ~2MB to ~4MB), I don't have tested it.
Watch is not working, see rollup fails to resolve entry file when using --watch, seemingly at random rollup/rollup#982

* official/master: (42 commits) chore: improve publish scripts Publish chore: test before publishing chore(netlify-cms-core): upgrade react-dnd to 7.x (decaporg#1922) chore(netlify-cms-widget-text): upgrade to react-textarea-autosize 7.x (decaporg#1921) chore(netlify-cms-core): upgrade to react-waypoint 8.x (decaporg#1920) fix(backend): use singular label in custom commit message (decaporg#1917) docs: add GAE-specific oAuth client (decaporg#1918) fix(netlify-cms-widget-text): set correct font family (decaporg#1916) chore(netlify-cms-core): upgrade redux and related dependencies (decaporg#1914) chore(dependencies): bump npm-run-all (decaporg#1913) fix(netlify-cms-core): fix identifier field validation (decaporg#1907) docs: improve widget custom validation sample (decaporg#1911) improvement(backends): changes for registerBackend compatibility (decaporg#1880) fix(netlify-cms-core): fix prop-types warnings (decaporg#1906) fix(a11y): correct label "for" references to fields (decaporg#1904) improvement(netlify-cms-core): wrap navigations in lists for better a11y (decaporg#1903) fix(netlify-cms-widget-markdown): add missing border radius on toolbar (decaporg#1905) fix(netlify-cms-core): remove double focusable elements on profile menu button (decaporg#1900) fix(config): remove identifier field validation (decaporg#1882) ...

* official/master: update release ticker Publish feat: add cloudinary support (decaporg#1932) fix(netlify-cms-core): duplicate key warning (decaporg#1930) Update image.md (decaporg#1923) chore(netlify-cms-core): upgrade react-frame-component to 4.x (decaporg#1925) chore(netlify-cms-core): upgrade gray-matter to 4.x (decaporg#1924) feat(netlify-cms-widget-select): add support for multiple selection (decaporg#1901) # Conflicts: # packages/netlify-cms-widget-select/package.json # packages/netlify-cms/src/media-libraries.js

alexandernanberg · 2018-12-05T17:09:00Z

packages/netlify-cms/webpack.config.js

-  entry: './index.js',
+  mode: isProduction ? 'production' : 'development',
+  context: path.join(__dirname, 'dist'),
+  entry: './netlify-cms.esm.js',


Is this really correct? I think webpack needs the raw source file. This could explain the large UMD bundle as well

erquhart · 2019-02-02T20:07:14Z

Closing this as abandoned, but please reopen if you'd like to move this forward.

erquhart · 2019-04-02T20:35:55Z

Update: this was addressed by @talves in #2214, #2141, and #2215.

LoicMahieu added 2 commits November 6, 2018 10:27

netlify-cms-core: remove alias and module-resolver

376ded3

Some advantages to remove aliases: - Better integration with code editors (VSCode in mind) - Easier to move to another bundler

First draft with rollup

4bdaf82

netlify-cms-backend-git-gateway: fix import BitbucketBackend

451c4b3

LoicMahieu force-pushed the feature/rollup branch from 74080c9 to cd1052a Compare November 6, 2018 15:27

Move NETLIFY_CMS_VERSION replacement to rollup instead of webpack

37377d4

LoicMahieu force-pushed the feature/rollup branch from c894742 to 2d7f248 Compare November 6, 2018 15:55

Cleanup webpack things, move it to netlify-cms and cleanup dependencies

63a2e23

LoicMahieu force-pushed the feature/rollup branch from 2d7f248 to 63a2e23 Compare November 6, 2018 16:10

LoicMahieu added 3 commits November 7, 2018 10:52

Revert "netlify-cms-core: remove alias and module-resolver"

33614aa

This reverts commit 376ded3. # Conflicts: # packages/netlify-cms-core/babel.config.js # packages/netlify-cms-core/src/reducers/auth.js

netlify-cms: don’t require /src but module himself

fbba542

netlify-cms-core: fix aliases and index files

194147f

For an unkonwn reason, some aliased import does not work in Rollup like Webpack. For example, `import ‘Redux’` results in `require(‘./redux/index.js’)` in the ESM bundle and it is not bundled.

LoicMahieu mentioned this pull request Nov 9, 2018

webpack: only process current sources #1867

Closed

LoicMahieu changed the title ~~WIP: rollup and ESM export~~ rollup and ESM export Nov 9, 2018

erquhart suggested changes Nov 22, 2018

View reviewed changes

erquhart mentioned this pull request Nov 22, 2018

feat(netlify-cms-core): add basic route code-splitting #1889

Merged

Remove dep to-string-loader from specific packages

1ba5aa1

erquhart mentioned this pull request Dec 3, 2018

Unmet peer dependencies #1678

Open

erquhart changed the title ~~rollup and ESM export~~ [WIP] rollup and ESM export Dec 3, 2018

alexandernanberg reviewed Dec 5, 2018

View reviewed changes

netlify-cms-media-library-cloudinary: build with rollup

9292cb5

erquhart closed this Feb 2, 2019

erquhart added the abandoned label Feb 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] rollup and ESM export #1858

[WIP] rollup and ESM export #1858

LoicMahieu commented Nov 6, 2018

verythorough commented Nov 6, 2018 •

edited

Loading

verythorough commented Nov 6, 2018 •

edited

Loading

LoicMahieu commented Nov 6, 2018 •

edited

Loading

erquhart commented Nov 6, 2018

LoicMahieu commented Nov 7, 2018

erquhart commented Nov 7, 2018

erquhart commented Nov 7, 2018

Benaiah commented Nov 7, 2018

LoicMahieu commented Nov 8, 2018

LoicMahieu commented Nov 9, 2018

erquhart commented Nov 12, 2018

erquhart left a comment •

edited

Loading

erquhart Nov 22, 2018

LoicMahieu Nov 29, 2018

erquhart commented Nov 22, 2018

alexandernanberg commented Nov 23, 2018

erquhart commented Nov 23, 2018

alexandernanberg commented Nov 26, 2018

erquhart commented Nov 26, 2018

LoicMahieu commented Nov 29, 2018

alexandernanberg Dec 5, 2018

erquhart commented Feb 2, 2019

erquhart commented Apr 2, 2019

[WIP] rollup and ESM export #1858

[WIP] rollup and ESM export #1858

Conversation

LoicMahieu commented Nov 6, 2018

1. Summary

Important changes:

Advantages:

Fixes issues:

Breaking changes

2. Test plan

3. A picture of a cute animal

verythorough commented Nov 6, 2018 • edited Loading

verythorough commented Nov 6, 2018 • edited Loading

LoicMahieu commented Nov 6, 2018 • edited Loading

erquhart commented Nov 6, 2018

LoicMahieu commented Nov 7, 2018

erquhart commented Nov 7, 2018

erquhart commented Nov 7, 2018

Benaiah commented Nov 7, 2018

LoicMahieu commented Nov 8, 2018

LoicMahieu commented Nov 9, 2018

erquhart commented Nov 12, 2018

erquhart left a comment • edited Loading

Choose a reason for hiding this comment

erquhart Nov 22, 2018

Choose a reason for hiding this comment

LoicMahieu Nov 29, 2018

Choose a reason for hiding this comment

erquhart commented Nov 22, 2018

alexandernanberg commented Nov 23, 2018

erquhart commented Nov 23, 2018

alexandernanberg commented Nov 26, 2018

erquhart commented Nov 26, 2018

LoicMahieu commented Nov 29, 2018

alexandernanberg Dec 5, 2018

Choose a reason for hiding this comment

erquhart commented Feb 2, 2019

erquhart commented Apr 2, 2019

verythorough commented Nov 6, 2018 •

edited

Loading

verythorough commented Nov 6, 2018 •

edited

Loading

LoicMahieu commented Nov 6, 2018 •

edited

Loading

erquhart left a comment •

edited

Loading