feat: allow typescript file extension #1135
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
diervo
changed the title
This comment has been minimized.
This comment has been minimized.
pmdartus
requested changes
Mar 26, 2019
|
|
||
| export default function nodeModulesPaths( | ||
| basedir: string, | ||
| { moduleDirectory }: NodeModulesConfig = {} |
There was a problem hiding this comment.
We should avoid defaulting internal value when not needed.
Suggested change
| { moduleDirectory }: NodeModulesConfig = {} | |
| { moduleDirectory }: NodeModulesConfig |
| @@ -97,13 +112,12 @@ function resolveModules(modules, opts) { | |||
| } | |||
| } | |||
|
|
|||
| function resolveLwcNpmModules(options = {}) { | |||
| function resolveLwcNpmModules(options: ModuleResolverConfig = {}) { | |||
There was a problem hiding this comment.
Remove common js style import at the end of the file.
Suggested change
| function resolveLwcNpmModules(options: ModuleResolverConfig = {}) { | |
| export function resolveLwcNpmModules(options: ModuleResolverConfig = {}) { |
| @@ -97,13 +112,12 @@ function resolveModules(modules, opts) { | |||
| } | |||
| } | |||
|
|
|||
| function resolveLwcNpmModules(options = {}) { | |||
| function resolveLwcNpmModules(options: ModuleResolverConfig = {}) { | |||
There was a problem hiding this comment.
The ModuleResolverConfig should be normalized and we should initialize the options field to avoid doing branching in the resolver.
You can use Partial for this (https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-1.html#mapped-types).
export interface ModuleResolverConfig {
moduleDirectories: string[];
rootDir: string;
}
function resolveLwcNpmModules(options: Partial<ModuleResolverConfig> = {}) {
const normalizedOptions: ModuleResolverConfig = normalizeOptions(options);
}| describe('test typescript like bundle', () => { | ||
| test('test typescript extension', async () => { | ||
| const customConfig = { | ||
| name: 'typescript.ts', |
There was a problem hiding this comment.
name should not include extension it should not be a valid LWC module name.
|
|
||
| test('test typescript grammar should fail', async () => { | ||
| const customConfig = { | ||
| name: 'typescript-typed.ts', |
There was a problem hiding this comment.
name should not include extension it should not be a valid LWC module name.
|
@diervo Since the compiler doesn't provide pre-processor hooks to run |
added 2 commits
March 26, 2019 12:06
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Benchmark resultsBase commit: lwc-engine-benchmark
|
pmdartus
approved these changes
Mar 27, 2019
apapko
approved these changes
Mar 27, 2019
| @@ -42,7 +42,8 @@ module.exports = function postProcess({ types: t }) { | |||
|
|
|||
| function getBaseName({ file }) { | |||
| const classPath = file.opts.filename; | |||
| return basename(classPath, '.js'); | |||
| const ext = extname(classPath); | |||
There was a problem hiding this comment.
do we need to perform extension validation to only allow .js and .ts or should module-resolver fail on its own
| message: expect.stringContaining( | ||
| 'Failed to resolve import "./lib/foo" from "foo.js". Please add "lib/foo.js" file to the component folder.' | ||
| ), | ||
| message: expect.stringContaining('Failed to resolve'), |
There was a problem hiding this comment.
can we retain the original messages here? We've added logic to provide better error messages and to explicitly state the missing files in the error, therefore i'd like to make sure that all the dynamic peaces are tested.
added 2 commits
March 27, 2019 18:47
…nto dval/tsExtensionSupport
Benchmark resultsBase commit: lwc-engine-benchmark
|
Benchmark resultsBase commit: lwc-engine-benchmark
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
Refactor module resolver and compile to allow typescript extension
.ts(note that is just the extension we don't allow TS grammar).Also this PR migrates module-resolver to typescript preparing for future changes.