Integrate liferay-npm-scripts into Liferay Learn #91

rdai10 · 2020-09-29T23:48:31Z

The Liferay Learn project would like to use @liferay/npm-scripts to format its source code (scss, js, md, yml, and html in the form of jinja 2 templates).

Preliminary work has been done to incorporate the package into the Learn project, but it's only being used to format scss so far. There are a few bumps on the road:

The project uses a tooling called Sphinx that packages jquery out of the box. In the project's javascript files, there are references to the jquery $ that throws errors when using npm-scripts. Are there any tips for configurations that we can add to get around that? A way to avoid inlining  comments would be preferred.
There is a conscious effort to organize the project into /docs and /site folders at the root to create a separation between documentation material and its website source. However, we do want to be able to format all the md files in the /docs folder from inside the /site/homepage folder. I ran into issues defining an appropriate glob pattern to go up the directories for my npmscripts.config.js file. I came across feat: handling globbing internally for Prettier liferay-npm-tools#141 where a custom glob library was implemented for liferay-npm-scripts and hope to receive some help understanding what it expects.

Let me know if I can provide any clarification that will be helpful! Thanks!

The text was updated successfully, but these errors were encountered:

rdai10 · 2020-09-30T00:06:36Z

@jrhoun, feel free to chime in!

wincent · 2020-09-30T06:45:08Z

Thanks @rdai10.

I added one comment here:

You don't need the yarn here.

You also have a couple of options if you want to roll this out more "gently".

One is to use fix and check like you are here. Those are going to run all of ESLint, Prettier, stylelint, plus any additional "pre-flight" checks that @liferay/npm-scripts implements. This is obviously a higher barrier which may add some friction in return for more consistency/quality (at least, that's the theory).

The other is to use format and format:check (ie. liferay-npm-scripts format and liferay-npm-scripts format:check). This only runs Prettier, so the barrier to entry is lower and it is easier to turn on this lighter-weight check, and later on tighten things up by activating the broader check.

For example, I don't think it's Prettier which is throwing the error you're talking about here:

there are references to the jquery $ that throws errors when using npm-scripts.

It's probably coming from ESLint. Can you share the console output so that we can analyze it and confirm that hypothesis? If it is from ESLint, we don't need/want to use prettier-ignore directives; we'll need to add an .eslintrc.js that tells ESLint about the $ global, and then it will stop complaining about it.

I ran into issues defining an appropriate glob pattern to go up the directories

You can't really do any .. type stuff using Prettier or ESLint. They expect to run from a "root" (in your case, that's current /site/homepage) — and for example, a file like .prettierignore has to be at the root or it won't have any effect.

So, if you want to selectively format some of /site/homepage and some of /docs, you have to hoist the config up to the common ancestor (ie. the root will be /) and then selectively ignore the directories that you don't want to check. I can help with the globs once you've done that.

The other thing I think we'll need to do is teach the scripts to accept *.md and *.yml globs. We have a hard-coded list of file-types that we want to support, and we started with a small/restrictive list because we wanted to roll out the tooling in a very controlled way in liferay-portal (given that so many filetypes there are already handled by the Java Source Formatter). I've created an issue for this: #92

rdai10 · 2020-09-30T17:54:38Z

@wincent
You are correct that it's ESlint that's complaining about the jquery $. When I switch to running format:check I don't see the same errors.
Here is the output when executed with check:

liferay-learn/site/homepage/_static/js/page-alert.js
  27:17  error  Unexpected function expression.  prefer-arrow-callback
  43:3  error  '$' is not defined.  no-undef
  43:51  error  Unexpected function expression.  prefer-arrow-callback
  53:1  error  '$' is not defined.  no-undef

The project has been adjusted to using format and format:check. I think it's okay for now to go with the lightweight formatting option since it's not a javascript heavy project. It would benefit the team more to format the .md and also eventually the jinja templates.

I looked around, lots of people seem to be wanting a jinja plugin for prettier but nobody has written it yet. The closest I found was https://github.com/robertquitt/prettier-plugin-djangohtml but alas it is not a working version.

wincent · 2020-10-01T08:09:10Z

The other thing you can do if you want to turn ESLint back on is tell it that $ is a global.

Something like this in an .eslintrc.js file at the root:

module.exports = {
    globals: {
        $: true,
    },
};

If you don't want to force usage of arrow functions, you can add this too:

    rules: {
        'prefer-arrow-callback': 'off',
    },

(The reason we prefer arrows in liferay-portal is we have Babel running over everything, which means arrow functions get safely transpiled and don't break IE.)

rdai10 · 2020-10-01T21:03:22Z

Thanks @wincent ! I added the config at https://github.com/brianchandotcom/liferay-learn/pull/74 and everything works as expected.

wincent · 2020-10-02T10:31:23Z

So we just pushed @liferay/npm-scripts v33.1.0, which includes that PR #103 allowing it to format YAML and Markdown.

At this point, @rdai10, we can probably close this issue. The remaining wish-list item (formatting those Jinja2 templates which have ".html" extension) could be split into another issue — although given the cost (high) vs the relative reward (🤷‍♂️), it would probably not be something that we would be likely to prioritize in the short-term.

rdai10 · 2020-10-02T18:11:18Z

Hey @wincent
Thanks for working so quickly on this! I updated to the latest version here, but when attempting to run check, encountered this error locally:

> liferay-npm-scripts check

{ Error: Cannot find module 'typescript'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:636:15)
    at Function.Module._load (internal/modules/cjs/loader.js:562:25)
    at Module.require (internal/modules/cjs/loader.js:692:17)
    at require (internal/modules/cjs/helpers.js:25:18)
    at Object.<anonymous> (/home/rebecca/projects/liferay/liferay-learn/node_modules/@typescript-eslint/typescript-estree/dist/parser.js:30:25)
    at Module._compile (internal/modules/cjs/loader.js:778:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)
    at Module.load (internal/modules/cjs/loader.js:653:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
    at Function.Module._load (internal/modules/cjs/loader.js:585:3) code: 'MODULE_NOT_FOUND' }

github action also throws the same error:

Looks like there's unmet dependency?

wincent · 2020-10-05T07:27:51Z

Cool, thanks for reporting that. We use typescript in our devDependencies and that module already happens to be in liferay-portal, so it works there. This means we meed to promote it from devDependencies to dependencies to get it to work out-of-the-box in places which don't necessarily already have typescript. I'll do that today.

This isn't _strictly_ necessary, but it is an accurate description of the requirements of the project. We go with the range of "^3.6.3" to match the lowest range that we're currently using in any project (which is found in liferay-js-toolkit), to minimize the risk of introducing duplicates in a consuming project. If you have a ".ts" file in your project, you are going to have `typescript` in your `devDependencies`, and our `overrides` configuration is going to use `@typescript-eslint/parser`, which in turn is going to require `typescript`, transitively, in `@typescript-eslint/typescript-estree` here: https://github.com/typescript-eslint/typescript-eslint/blob/c5ffe8833da7875e82953de117b0abe6fa8a60b0/packages/typescript-estree/src/parser.ts#L2 And all will work, even though `@typescript-eslint/typescript-estree` doesn't explicitly declare its hard dependency on `typescript` in its package.json: https://github.com/typescript-eslint/typescript-eslint/blob/c5ffe8833da7875e82953de117b0abe6fa8a60b0/packages/typescript-estree/package.json#L41-L49 But if you don't have any ".ts", then you shouldn't need to have `typescript` in your dependency graph, and we won't use `@typescript/eslint-parser`. So, it makes sense for us to call this an `peerDependency` and mark it as `optional` as per these posts: - https://medium.com/@noamgjacobsonknzi/what-is-peerdependenciesmeta-acea887c32dd - npm/cli#1247 (Note that the `peerDependenciesMeta` field isn't documented yet.) You could also argue that this should be under `optionalDependencies`: https://docs.npmjs.com/files/package.json#optionaldependencies But I think "optional peer" comes closer to the reality here. `optionalDependencies` is for things that you can add, but the package should work if they're not there. That's not the case here. The package won't work if you have ".ts" files but no `typescript`. Related: #91 (comment)

This isn't _strictly_ necessary, but it is an accurate description of the requirements of the project. We go with the somewhat unsatisfying range of "*" to to minimize the risk of introducing duplicates in a consuming project. At the moment, we're using v4+ in this monorepo, 3.9.7 in liferay-portal, and the lowest range that we're currently using in any other project is "^3.6.3" (in liferay-js-toolkit). In the future, we should be able to converge on v4+. If you have a ".ts" file in your project, you are going to have `typescript` in your `devDependencies`, and our `overrides` configuration is going to use `@typescript-eslint/parser`, which in turn is going to require `typescript`, transitively, in `@typescript-eslint/typescript-estree` here: https://github.com/typescript-eslint/typescript-eslint/blob/c5ffe8833da7875e82953de117b0abe6fa8a60b0/packages/typescript-estree/src/parser.ts#L2 And all will work, even though `@typescript-eslint/typescript-estree` doesn't explicitly declare its hard dependency on `typescript` in its package.json: https://github.com/typescript-eslint/typescript-eslint/blob/c5ffe8833da7875e82953de117b0abe6fa8a60b0/packages/typescript-estree/package.json#L41-L49 But if you don't have any ".ts", then you shouldn't need to have `typescript` in your dependency graph, and we won't use `@typescript/eslint-parser`. So, it makes sense for us to call this an `peerDependency` and mark it as `optional` as per these posts: - https://medium.com/@noamgjacobsonknzi/what-is-peerdependenciesmeta-acea887c32dd - npm/cli#1247 (Note that the `peerDependenciesMeta` field isn't documented yet.) You could also argue that this should be under `optionalDependencies`: https://docs.npmjs.com/files/package.json#optionaldependencies But I think "optional peer" comes closer to the reality here. `optionalDependencies` is for things that you can add, but the package should work if they're not there. That's not the case here. The package won't work if you have ".ts" files but no `typescript`. Related: #91 (comment)

As reported here: #91 (comment) Our use of the `@typescript-eslint/parser` (since 50a4d99) means that we have a runtime dependency on `typescript` package. This worked fine in liferay-portal, where `typescript` is already present, but not in liferay-learn, where it is not. And I didn't know that `@typescript-eslint/parser` doesn't actually declare all of its transitive dependencies; as noted in: #113 `@typescript-eslint/typescript-estree` does an unguarded `import` of `typescript` even though it is not declared in its dependencies (they declare it as an optional peer dependency). So, let's not only fix this, by explicitly declaring our dependency on TypeScript, but also declare all of our TS-related dependencies. This will allow us to delete the devDependencies currently declared here: https://github.com/liferay/liferay-portal/blob/f1a2b2ce06fa050d5e1ab193728ab9ec06df4315/modules/apps/remote-app/remote-app-client-js/package.json#L3-L6

wincent · 2020-10-05T11:35:26Z

We just pushed https://github.com/liferay/liferay-frontend-projects/releases/tag/npm-scripts%2Fv33.1.2 which should include the necessary typescript dependency.

rdai10 · 2020-10-05T17:34:07Z

Thanks @wincent for all your help! Applied the changes at https://github.com/jrhoun/liferay-learn/pull/701 and all seems to be working!

wincent · 2020-10-06T07:08:47Z

Cool. Thanks for confirming @rdai10.

wincent mentioned this issue Sep 30, 2020

Allow npm-scripts to format .md and .yml files #92

Closed

wincent added feat npm-scripts question labels Sep 30, 2020

wincent mentioned this issue Oct 5, 2020

chore(eslint-config): mark typescript as a peer dependency #113

Merged

wincent mentioned this issue Oct 5, 2020

fix(npm-scripts): bundle TypeScript dependencies #114

Merged

rdai10 closed this as completed Oct 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate liferay-npm-scripts into Liferay Learn #91

Integrate liferay-npm-scripts into Liferay Learn #91

rdai10 commented Sep 29, 2020 •

edited

Loading

rdai10 commented Sep 30, 2020

wincent commented Sep 30, 2020

rdai10 commented Sep 30, 2020 •

edited

Loading

wincent commented Oct 1, 2020

rdai10 commented Oct 1, 2020

wincent commented Oct 2, 2020

rdai10 commented Oct 2, 2020

wincent commented Oct 5, 2020

wincent commented Oct 5, 2020

rdai10 commented Oct 5, 2020

wincent commented Oct 6, 2020

Integrate liferay-npm-scripts into Liferay Learn #91

Integrate liferay-npm-scripts into Liferay Learn #91

Comments

rdai10 commented Sep 29, 2020 • edited Loading

rdai10 commented Sep 30, 2020

wincent commented Sep 30, 2020

rdai10 commented Sep 30, 2020 • edited Loading

wincent commented Oct 1, 2020

rdai10 commented Oct 1, 2020

wincent commented Oct 2, 2020

rdai10 commented Oct 2, 2020

wincent commented Oct 5, 2020

wincent commented Oct 5, 2020

rdai10 commented Oct 5, 2020

wincent commented Oct 6, 2020

rdai10 commented Sep 29, 2020 •

edited

Loading

rdai10 commented Sep 30, 2020 •

edited

Loading