Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

chore(mixed): fix imports and introduce bundler test #570

Merged
merged 18 commits into from
Dec 11, 2018
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ jobs:
# - run:
Copy link
Contributor

@kuzhelov kuzhelov Dec 10, 2018

Choose a reason for hiding this comment

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

when we are planning to return this? And, actually, what was specific reason to disable these tests - currently we have no any checks that any client is able to consume our library

Copy link
Member Author

Choose a reason for hiding this comment

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

After #550 will be merged

# name: Project Tests
# command: yarn test:projects:cra-ts
- run:
name: Bundler Tests
command: yarn test:bundlers:rollup
Copy link
Contributor

Choose a reason for hiding this comment

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

would suggest test:projects:rollup - as this won't introduce additional entropy and will still suggest that stack that client is using for her project

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved into single entry, yarn test:projects 👍

- run:
name: Danger JS
command: |
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
- The border color of the Icon is inherited if no value is provided for the `color` and `borderColor` variables @mnajdova ([#569](https://github.com/stardust-ui/react/pull/569))
- Do not focus `Popup`'s trigger on outside click @sophieH29 ([#578](https://github.com/stardust-ui/react/pull/578))
- Add `https` protocol to all urls used in the scripts and stylesheets in index.ejs @mnajdova ([#571](https://github.com/stardust-ui/react/pull/571))
- Fix issue with bundling packagewith Rollup and Parcel @layershifter ([#570](https://github.com/stardust-ui/react/pull/570))

### Features
- `Ref` components uses `forwardRef` API by default @layershifter ([#491](https://github.com/stardust-ui/react/pull/491))
Expand Down
12 changes: 12 additions & 0 deletions build/gulp/scaffold/app.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import React from 'react'
import ReactDOM from 'react-dom'
import { Button, Provider, themes } from '@stardust-ui/react'

ReactDOM.render(
React.createElement(
Provider,
{ theme: themes.teams },
React.createElement(Button, { content: 'Theming' }),
),
document.getElementById('root'),
)
110 changes: 110 additions & 0 deletions build/gulp/scaffold/rollup.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
import replace from 'rollup-plugin-replace'
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be placed somewhere under test-projects at the very least (we should create corresponding dir) - so that this client-oriented tests functionality will be cohesively structured

Copy link
Member Author

Choose a reason for hiding this comment

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

It's already placed in the separate dir. I don't want to be so specific for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, but the same reasoning would apply well in case it will be put in any dir - thus, I am not sure that this is a valid argument.

The reasoning behind my ask is the following: currently, with the dir introduced at the place where you are suggesting, we would have the following project structure:

/build/gulp
  /scaffold
  /tasks
       /test-projects

So, reasonable question is why scaffold directory is placed at this level of folder hierarchy? Is its content shared by any agents/tasks? The answer is "no" - the only place where it is used is test-projects module. Thus, I would suggest to put these bits to the module those belong to at the moment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is its content shared by any agents/tasks? The answer is "no" - the only place where it is used is test-projects module.

At now, yes. But in the future, I think that we will have other tasks that will should also have files and templates.

Copy link
Contributor

@mnajdova mnajdova Dec 11, 2018

Choose a reason for hiding this comment

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

I must agree here that better directory structure would reflect exactly what we test even. Having structure, something like:

/build/gulp
  /tasks
       /test-projects
         /cra-ts
         /parcel
         ...

will be much more cleaner, and it will be obvious what scenarios we got covered so far...

Copy link
Member Author

Choose a reason for hiding this comment

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

@kuzhelov @mnajdova you convinced me and I made these changes 👍

import resolve from 'rollup-plugin-node-resolve'
import commonjs from 'rollup-plugin-commonjs'

const lodashExports = [
Copy link
Member

@miroslavstastny miroslavstastny Dec 10, 2018

Choose a reason for hiding this comment

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

(!) Missing exports
https://rollupjs.org/guide/en#error-name-is-not-exported-by-module-
node_modules/@stardust-ui/react/dist/es/components/Dropdown/Dropdown.js
pickBy is not exported by node_modules/lodash/lodash.js

How is this going to scale? Whenever a new function is used, do I need to remember I have to add it here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's a real problem 👎 lodash is CJS module, so the there is only one option.
If we will use Babel and babel-plugin-lodash this problem will gone.

Copy link
Member Author

Choose a reason for hiding this comment

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

Resolved with external and output.globals, now is much cleaner

'compact',
'difference',
'each',
'findIndex',
'flow',
'forEach',
'get',
'has',
'filter',
'first',
'includes',
'intersection',
'invoke',
'isArray',
'isEmpty',
'isFunction',
'isNil',
'isObject',
'isPlainObject',
'inRange',
'isUndefined',
'keys',
'last',
'map',
'mapValues',
'merge',
'memoize',
'min',
'pick',
'round',
'set',
'some',
'sortBy',
'startsWith',
'sum',
'take',
'trim',
'without',
'union',
'uniq',
'uniqueId',
]

export default {
input: 'app.js',
output: {
file: 'bundle.js',
format: 'iife',
sourcemap: true,
},
plugins: [
replace({
'process.env.NODE_ENV': JSON.stringify('development'),
}),
resolve(),
commonjs({
include: 'node_modules/**',
namedExports: {
'node_modules/keyboard-key/src/keyboardKey.js': [
'ArrowDown',
'ArrowUp',
'ArrowLeft',
'ArrowRight',
'End',
'Enter',
'Escape',
'Home',
'getCode',
'Spacebar',
'Tab',
],
'node_modules/lodash/fp.js': lodashExports,
'node_modules/lodash/lodash.js': lodashExports,
'node_modules/prop-types/index.js': [
'any',
'arrayOf',
'bool',
'element',
'func',
'node',
'number',
'object',
'oneOf',
'oneOfType',
'shape',
'string',
'symbol',
],
'node_modules/react/index.js': [
'Component',
'cloneElement',
'createRef',
'PureComponent',
'Fragment',
'Children',
'createElement',
'isValidElement',
],
'node_modules/react-dom/index.js': ['createPortal', 'findDOMNode'],
'node_modules/react-is/index.js': ['isForwardRef'],
'node_modules/what-input/dist/what-input.js': ['ask'],
},
}),
],
}
12 changes: 11 additions & 1 deletion build/gulp/tasks/dist.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import * as rimraf from 'rimraf'
import * as webpack from 'webpack'

import config from '../../../config'
import importsTransformer from '../../ts/transformer-namespace-imports'

const { paths } = config
const g = require('gulp-load-plugins')()
Expand Down Expand Up @@ -38,7 +39,16 @@ task('build:dist:commonjs', () => {
task('build:dist:es', () => {
const tsConfig = paths.base('build/tsconfig.es.json')
const settings = { declaration: true }
const typescript = g.typescript.createProject(tsConfig, settings)
const typescript = g.typescript.createProject(tsConfig, {
...settings,
getCustomTransformers: () => ({
before: [
importsTransformer({
moduleNames: ['classnames', 'color'],
}),
],
}),
})

const { dts, js } = src(paths.src('**/*.{ts,tsx}')).pipe(typescript())
const types = src(paths.base('types/**'))
Expand Down
47 changes: 47 additions & 0 deletions build/gulp/tasks/test-bundlers.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import * as debug from 'debug'
import { task } from 'gulp'
import * as fs from 'fs'
import * as path from 'path'
import * as tmp from 'tmp'

import { buildAndPackStardust, createPackageFilename, runIn } from './test-projects'

const logger = debug('bundle:rollup')
logger.enabled = true

task('test:bundlers:rollup', async () => {
const packageFilename = createPackageFilename()

await buildAndPackStardust(packageFilename)
logger(`✔️Stardust UI package was prepared: ${packageFilename}`)

const tmpDirectory = tmp.dirSync({ prefix: 'stardust-' }).name
logger(`✔️Temporary directory was created: ${tmpDirectory}`)

const dependencies = [
'rollup',
'rollup-plugin-replace',
'rollup-plugin-commonjs',
'rollup-plugin-node-resolve',
'react',
'react-dom',
].join(' ')
await runIn(tmpDirectory)(`yarn add ${dependencies}`)
logger(`✔️Dependencies were installed`)

await runIn(tmpDirectory)(`yarn add ${packageFilename}`)
logger(`✔️Stardust UI was added to dependencies`)

fs.copyFileSync(
path.resolve(__dirname, '..', 'scaffold', 'app.js'),
path.resolve(tmpDirectory, 'app.js'),
)
fs.copyFileSync(
path.resolve(__dirname, '..', 'scaffold', 'rollup.config.js'),
path.resolve(tmpDirectory, 'rollup.config.js'),
)
logger(`✔️Source and bundler's config were created`)

await runIn(tmpDirectory)(`yarn rollup -c`)
logger(`✔️Example project was successfully built: ${tmpDirectory}`)
Copy link
Member

Choose a reason for hiding this comment

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

It seems we are ignoring rollup warnings. Are we ok with it or would we like to use this as an opportunity to tighten our rules and require warning-free build in rollup? (no warnings are currently allowed in webpack build)

Copy link
Member Author

Choose a reason for hiding this comment

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

I reworked it, now we have a warning whitelist, other warnings will throw.

We can fix all left circular imports in a separate PR because it will require more work.

})
19 changes: 11 additions & 8 deletions build/gulp/tasks/test-projects.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,13 @@ const log = msg => {
console.log('='.repeat(80))
}

const runIn = path => cmd => sh(`cd ${path} && ${cmd}`)
export const createPackageFilename = () => tmp.tmpNameSync({ prefix: 'stardust-', postfix: '.tgz' })

const buildAndPackStardust = async (): Promise<string> => {
await sh('yarn build:dist')
export const runIn = path => cmd => sh(`cd ${path} && ${cmd}`)

return (await sh(`npm pack`, true)).trim()
export const buildAndPackStardust = async (packageFilename: string) => {
await sh('yarn build:dist')
await sh(`yarn pack --filename ${packageFilename}`)
}

const createReactApp = async (atTempDirectory: string, appName: string): Promise<string> => {
Expand Down Expand Up @@ -90,8 +91,10 @@ export default App;
//////// PREPARE STARDUST PACKAGE ///////
log('STEP 0. Preparing Stardust package..')

const stardustPackageFilename = await buildAndPackStardust()
log(`Stardust package is published: ${paths.base(stardustPackageFilename)}`)
const packageFilename = createPackageFilename()

await buildAndPackStardust(packageFilename)
log(`Stardust package is published: ${packageFilename}`)

try {
//////// CREATE TEST REACT APP ///////
Expand All @@ -107,7 +110,7 @@ export default App;
//////// ADD STARDUST AS A DEPENDENCY ///////
log('STEP 2. Add Stardust dependency to test project..')

await runInTestApp(`yarn add ${paths.base(stardustPackageFilename)}`)
await runInTestApp(`yarn add ${packageFilename}`)
log("Stardust is successfully added as test project's dependency.")

//////// REFERENCE STARDUST COMPONENTS IN TEST APP's MAIN FILE ///////
Expand All @@ -120,6 +123,6 @@ export default App;

log('Test project is built successfully!')
} finally {
fs.unlinkSync(stardustPackageFilename)
fs.unlinkSync(packageFilename)
}
})
59 changes: 59 additions & 0 deletions build/ts/transformer-namespace-imports.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import * as ts from 'typescript'

type Options = {
moduleNames: string[]
}

/**
* A factory that creates ES default import, i.e: import _ from 'lodash'
*/
const createDefaultImport = (importName: string, moduleName: string) =>
ts.createImportDeclaration(
undefined,
undefined,
ts.createImportClause(ts.createIdentifier(importName), undefined),
ts.createStringLiteral(moduleName),
)

/**
* A functions transforms ES import to default if it's required:
* import * as _ from 'lodash' - will be transformed
* import { pick } from 'lodash' - will be skipped
* import _ from 'lodash' - will be skipped
*/
const transformImportIfRequired = (
node: ts.ImportDeclaration,
moduleNames: string[],
): ts.ImportDeclaration => {
const importClause = node.importClause
// Heads up!
// We need there slice quoutes: 'color' => color
const importName = node.moduleSpecifier.getText().slice(1, -1)
const includes = moduleNames.indexOf(importName) !== -1

if (includes && ts.isNamespaceImport(importClause.namedBindings)) {
return createDefaultImport(importClause.namedBindings.name.text, importName)
}

return node
}

const createTransformer = (
options: Partial<Options> = {},
): ts.TransformerFactory<ts.SourceFile> => {
return (context: ts.TransformationContext) => {
return (node: ts.SourceFile) => {
const visitor: ts.Visitor = (node: ts.Node) => {
if (ts.isImportDeclaration(node)) {
return transformImportIfRequired(node, options.moduleNames)
}

return ts.visitEachChild(node, visitor, context)
}

return ts.visitNode(node, visitor)
}
}
}

export default createTransformer
1 change: 1 addition & 0 deletions gulpfile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ require('./build/gulp/tasks/screener')
require('./build/gulp/tasks/git')
require('./build/gulp/tasks/test-unit')
require('./build/gulp/tasks/test-projects')
require('./build/gulp/tasks/test-bundlers')

// global tasks
task('build', series('dll', parallel('dist', 'build:docs')))
7 changes: 4 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
"scripts": {
"build": "gulp build",
"build:docs": "gulp --series dll build:docs",
"build:dist": "gulp --series dll build:dist",
"build:dist": "gulp --series build:dist",
"ci": "yarn lint && yarn prettier && yarn test --strict",
"predeploy:docs": "cross-env NODE_ENV=production yarn build:docs",
"deploy:docs": "gulp deploy:docs",
Expand All @@ -38,6 +38,7 @@
"test:vulns": "snyk test",
"test:visual": "gulp screener",
"test:projects:cra-ts": "gulp test:projects:cra-ts",
"test:bundlers:rollup": "gulp test:bundlers:rollup",
"generate:component": "gulp generate:component"
},
"lint-staged": {
Expand Down Expand Up @@ -75,7 +76,7 @@
},
"devDependencies": {
"@babel/standalone": "^7.1.0",
"@types/classnames": "^2.2.4",
"@types/classnames": "^2.2.6",
"@types/color": "^3.0.0",
"@types/enzyme": "^3.1.14",
"@types/faker": "^4.1.3",
Expand Down Expand Up @@ -113,7 +114,7 @@
"gulp-rename": "^1.3.0",
"gulp-replace": "^1.0.0",
"gulp-transform": "^3.0.5",
"gulp-typescript": "^5.0.0-alpha.1",
"gulp-typescript": "^5.0.0",
"gulp-util": "^3.0.8",
"html-webpack-plugin": "^3.2.0",
"husky": "^0.14.3",
Expand Down
4 changes: 3 additions & 1 deletion src/components/Avatar/Avatar.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import * as PropTypes from 'prop-types'
import * as React from 'react'
import { Image, Label, Status } from '../../'
import Image from '../Image/Image'
import Label from '../Label/Label'
import Status from '../Status/Status'
import { Extendable, ShorthandValue } from '../../../types/utils'
import {
createShorthandFactory,
Expand Down
4 changes: 3 additions & 1 deletion src/components/Label/Label.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ import {
commonPropTypes,
} from '../../lib'

import { Icon, Image, Layout } from '../..'
import Icon from '../Icon/Icon'
import Image from '../Image/Image'
import Layout from '../Layout/Layout'
import { Accessibility } from '../../lib/accessibility/types'
import { Extendable, ShorthandValue } from '../../../types/utils'

Expand Down
Loading