[NC-438] PIW-tools: Optimise rollup build process #926

wegiswes · 2021-09-17T10:16:09Z

This PR contains

What is the purpose of this PR?

This PR adds a formatting step to rolup. This will prettify the unpretty, auto-generated typings files.
This PR also includes a feature where you can use MX_PROJECT_PATH in combination with npm run release.

What should be covered while testing?

We should build all Native & Web widgets with this change to see if the error above is actually fixed.

packages/tools/pluggable-widgets-tools/configs/rollup.config.js

scripts/release/copyReleaseToProject.js

Dismissing my review in order to allow native to use their own rollup file.

packages/tools/pluggable-widgets-tools/configs/rollup.config.native.js

tamelbrun

Please use pluggable-widget-tools in the commit message when a commit touches the tooling, otherwise we cannot easily identify them.

tamelbrun · 2021-09-27T14:25:57Z

packages/tools/pluggable-widgets-tools/configs/rollup.config.native.js

@@ -194,6 +195,7 @@ export default async args => {
    function getClientComponentPlugins() {
        return [
            isTypescript ? widgetTyping({ sourceDir: join(sourcePath, "src") }) : null,
+            execShellCommand(`npx pluggable-widgets-tools format`, sourcePath),


The PR description does not mention anything about formatting. Why do we need this step? Don't we already have Prettier configured by default as a pre-release step? To me, it feels rather intrusive for customers if the pluggable widget tools start formatting unconditionally without an easy way to override it.

Sorry I forgot to update the description. Lots of things have changed regarding rollup lately.

The reason for this step is that our generated type files are not pretty. This results in a lot of unwanted changes and means the developer has to either discard them, or include them in their commit (so it's fixed by a pre-commit hook which includes prettier).
I don't like either of those "solutions" (discard / pre-commit). Therefore I added this format step.
I can see that it could be intrusive for customers. Maybe we should add another env variable for our internal use, all though I imagine a lot of customers are running into the same issue if they prettify their codebase. What do you think would be the best solution?

At the very least, I would restrict the reformatting to the typings only. This is the solution that Isa chose for web, you can take a look at his PR.

I couldn't find Isa's solution, but I've updated the code so it only targets the typings/ folder.

https://github.com/mendix/widgets-resources/pull/941/files

I've also suggested in that PR that this should probably be part of the widgetTyping plugin itself, in which case you would not need a separate solution here. In general, this kind of behaviour should be kept in sync between web and native, so that would be a good way to achieve that :).

I agree, although I didn't expect web to apply a similar solution in the end. One of the reasons why we split rollup is because native wanted this feature while web did not.. 😕

Anyhow I updated my code so formatting is now part of widgetTyping 👍🏼

Did you coordinate with Isa? Because if he now tries to do the same thing, one of you will get a merge conflict on the widgetTyping plugin.

I think that web definitely did not want a reformat on the entire project, which is what happened before :). I can see other advantages in splitting up the rollup config for web and native, but we definitely shouldn't introduce arbitrary divergence between the two.

Everything from #941 is taken care of in this PR now.

diego-antonelli · 2021-09-29T09:23:59Z

packages/tools/pluggable-widgets-tools/configs/rollup-plugin-widget-typing.js

@@ -23,6 +23,10 @@ export function widgetTyping({ sourceDir }) {
            if (!firstRun) {
                await runTransformation(sourceDir);
            }
+            await execShellCommand(
+                `npx pluggable-widgets-tools format:custom-paths -- "../typings/**/*.ts"`,


Can we be more strict and only format the file generated by this function? You have the output, so its easy to reuse it.

Good one. You could let the transformPackage function return the destination path and use that to format the generated typing file.

IIsaku

Nice work Wesley! I made a few remarks for you to have a look at.

packages/tools/pluggable-widgets-tools/bin/mx-scripts.js

IIsaku · 2021-09-30T08:02:30Z

packages/tools/pluggable-widgets-tools/configs/rollup-plugin-widget-typing.js

@@ -23,6 +23,10 @@ export function widgetTyping({ sourceDir }) {
            if (!firstRun) {
                await runTransformation(sourceDir);
            }
+            await execShellCommand(
+                `npx pluggable-widgets-tools format:custom-paths -- "../typings/**/*.ts"`,


Good one. You could let the transformPackage function return the destination path and use that to format the generated typing file.

packages/tools/pluggable-widgets-tools/configs/rollup-plugin-widget-typing.js

packages/tools/pluggable-widgets-tools/src/typings-generator/index.ts

IIsaku

Nice work, two points to consider.

packages/tools/pluggable-widgets-tools/CHANGELOG.md

tamelbrun · 2021-10-01T09:58:48Z

Please use pluggable-widget-tools in the commit message when a commit touches the tooling, otherwise we cannot easily identify them.

@wesleysieses Can you change the commit messages in this PR accordingly? I noticed that some new ones without the tag were added.

wegiswes · 2021-10-01T12:41:26Z

Please use pluggable-widget-tools in the commit message when a commit touches the tooling, otherwise we cannot easily identify them.

@wesleysieses Can you change the commit messages in this PR accordingly? I noticed that some new ones without the tag were added.

@tamelbrun Should be fixed now, thanks! 👌🏼

tamelbrun · 2021-10-12T15:27:14Z

packages/tools/pluggable-widgets-tools/configs/rollup-plugin-widget-typing.js

+                `npx pluggable-widgets-tools format:custom-files -- ${propsTypingFilePaths
+                    .map(f =>
+                        platform() === "win32"
+                            ? `${f.replace(/(?<=[/\\])([^/\\]* [^/\\]*)(?=[/\\])/g, '"$1"')}`


I have two minor comments:

The string template does not appear to be necessary.

Given that regexes can be notoriously hard to read, extracting this to a small quoteFoldersWithSpaces function would make this code document itself.

Nice regex, by the way 😉

tamelbrun · 2021-10-12T15:29:33Z

packages/tools/pluggable-widgets-tools/configs/rollup-plugin-widget-typing.js

+                    .map(f =>
+                        platform() === "win32"
+                            ? `${f.replace(/(?<=[/\\])([^/\\]* [^/\\]*)(?=[/\\])/g, '"$1"')}`
+                            : `'"${f}"'`


The inner string is now both double-quoted and single-quoted in the output on non-Windows platforms, that doesn't seem right?

Co-authored-by: I. Isaku <16718722+IIsaku@users.noreply.github.com>

wegiswes added enhancement New feature or request Cross-Platform Web & Native labels Sep 17, 2021

wegiswes requested a review from a team September 17, 2021 10:16

wegiswes requested a review from a team as a code owner September 17, 2021 10:16

wegiswes changed the title ~~Fix Studio Pro ReferenceError related to rollup terser~~ [NC-438] Fix Studio Pro ReferenceError related to rollup terser Sep 17, 2021

diego-antonelli reviewed Sep 17, 2021

View reviewed changes

packages/tools/pluggable-widgets-tools/configs/rollup.config.js Outdated Show resolved Hide resolved

jordanch suggested changes Sep 21, 2021

View reviewed changes

packages/tools/pluggable-widgets-tools/configs/rollup.config.js Outdated Show resolved Hide resolved

uicontent force-pushed the fix/npm-release-error branch from ae646b9 to cd92aa6 Compare September 21, 2021 08:57

wegiswes requested review from jordanch and diego-antonelli September 21, 2021 09:10

wegiswes changed the title ~~[NC-438] Fix Studio Pro ReferenceError related to rollup terser~~ [NC-438] Optimise rollup build process Sep 21, 2021

diego-antonelli reviewed Sep 21, 2021

View reviewed changes

packages/tools/pluggable-widgets-tools/configs/rollup.config.js Outdated Show resolved Hide resolved

diego-antonelli previously requested changes Sep 21, 2021

View reviewed changes

scripts/release/copyReleaseToProject.js Outdated Show resolved Hide resolved

jordanch reviewed Sep 23, 2021

View reviewed changes

scripts/release/copyReleaseToProject.js Outdated Show resolved Hide resolved

scripts/release/copyReleaseToProject.js Outdated Show resolved Hide resolved

uicontent force-pushed the fix/npm-release-error branch from c638dbe to 01b6ec9 Compare September 23, 2021 10:05

jordanch previously approved these changes Sep 23, 2021

View reviewed changes

uicontent dismissed jordanch’s stale review via a548d23 September 23, 2021 10:20

uicontent force-pushed the fix/npm-release-error branch from 01b6ec9 to a548d23 Compare September 23, 2021 10:20

uicontent force-pushed the fix/npm-release-error branch from 93e1ab5 to 69664ea Compare September 24, 2021 11:07

jordanch reviewed Sep 24, 2021

View reviewed changes

packages/tools/pluggable-widgets-tools/configs/rollup.config.native.js Outdated Show resolved Hide resolved

tamelbrun suggested changes Sep 27, 2021

View reviewed changes

tamelbrun reviewed Sep 27, 2021

View reviewed changes

wegiswes changed the title ~~[NC-438] Optimise rollup build process~~ [NC-438] PIW-tools: Optimise rollup build process Sep 28, 2021

wegiswes force-pushed the fix/npm-release-error branch from 69664ea to c743539 Compare September 28, 2021 15:37

diego-antonelli suggested changes Sep 29, 2021

View reviewed changes

IIsaku reviewed Sep 30, 2021

View reviewed changes

packages/tools/pluggable-widgets-tools/configs/rollup-plugin-widget-typing.js Outdated Show resolved Hide resolved

IIsaku reviewed Sep 30, 2021

View reviewed changes

packages/tools/pluggable-widgets-tools/src/typings-generator/index.ts Outdated Show resolved Hide resolved

IIsaku reviewed Sep 30, 2021

View reviewed changes

packages/tools/pluggable-widgets-tools/CHANGELOG.md Show resolved Hide resolved

IIsaku mentioned this pull request Sep 30, 2021

chore(pluggable-widgets-tools): improve prettifying functionality for web #941

Closed

5 tasks

wegiswes force-pushed the fix/npm-release-error branch 2 times, most recently from 265d8c9 to bf65882 Compare October 1, 2021 12:41

wegiswes force-pushed the fix/npm-release-error branch 4 times, most recently from ddcf07a to 8a976c5 Compare October 12, 2021 13:45

tamelbrun suggested changes Oct 12, 2021

View reviewed changes

undefined and others added 12 commits October 21, 2021 10:53

feat(pluggable-widgets-tools): add script to copy release to project

fdd1130

feat(pluggable-widgets-tools): improve rollup build process

b22ac10

fix(pluggable-widgets-tools): limit format to typings

6f53eb5

fix(pluggable-widgets-tools): move format to shared widget-typing

372ba7d

feat(pluggable-widgets-tools): add changelog

ab6e29c

fix(pluggable-widgets-tools): use generated type paths as prettier input

ed88581

fix(pluggable-widgets-tools): update format cmd name

cca1391

fix(pluggable-widgets-tools): update variable names

1a33dec

Update packages/tools/pluggable-widgets-tools/CHANGELOG.md

cc65674

Co-authored-by: I. Isaku <16718722+IIsaku@users.noreply.github.com>

fix(pluggable-widgets-tools): add double quotes around path

0526814

fix(pluggable-widgets-tools): wrap spaced folders on windows

5be444d

fix(pluggable-widgets-tools): duplicate path separators on windows

7e61010

wegiswes force-pushed the fix/npm-release-error branch from 3098a09 to 7e61010 Compare October 21, 2021 08:55

diego-antonelli closed this Feb 1, 2022

iobuhov deleted the fix/npm-release-error branch August 26, 2022 19:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NC-438] PIW-tools: Optimise rollup build process #926

[NC-438] PIW-tools: Optimise rollup build process #926

wegiswes commented Sep 17, 2021 •

edited

tamelbrun left a comment

tamelbrun Sep 27, 2021

wegiswes Sep 28, 2021

tamelbrun Sep 28, 2021

wegiswes Sep 28, 2021

diego-antonelli Sep 28, 2021

tamelbrun Sep 28, 2021

wegiswes Sep 29, 2021

tamelbrun Sep 29, 2021

IIsaku Sep 30, 2021

diego-antonelli Sep 29, 2021

IIsaku Sep 30, 2021

IIsaku left a comment

IIsaku Sep 30, 2021

IIsaku left a comment

tamelbrun commented Oct 1, 2021

wegiswes commented Oct 1, 2021 •

edited

tamelbrun Oct 12, 2021 •

edited

tamelbrun Oct 12, 2021

tamelbrun Oct 12, 2021

[NC-438] PIW-tools: Optimise rollup build process #926

[NC-438] PIW-tools: Optimise rollup build process #926

Conversation

wegiswes commented Sep 17, 2021 • edited

This PR contains

What is the purpose of this PR?

What should be covered while testing?

tamelbrun 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

IIsaku left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IIsaku left a comment

Choose a reason for hiding this comment

tamelbrun commented Oct 1, 2021

wegiswes commented Oct 1, 2021 • edited

tamelbrun Oct 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wegiswes commented Sep 17, 2021 •

edited

wegiswes commented Oct 1, 2021 •

edited

tamelbrun Oct 12, 2021 •

edited