Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP fix(bundler): fix commonjs node id compatibility. #831

Closed
wants to merge 1 commit into from
Closed

WIP fix(bundler): fix commonjs node id compatibility. #831

wants to merge 1 commit into from

Conversation

3cp
Copy link
Member

@3cp 3cp commented Feb 27, 2018

Commonjs thinks "foo" and "foo.js" are same id, but AMD thinks they are two different ids. Passing nodeIdCompat: true to requirejs config to improve compatibility to whole range of commonjs packages. Ref: amodrojs/amodro-trace#7

Commonjs thinks "foo" and "foo.js" are same id, but AMD thinks they are two different ids. Passing `nodeIdCompact: true` to requirejs config to improve compatibility to whole range of commonjs packages. Ref: amodrojs/amodro-trace#7
@3cp
Copy link
Member Author

3cp commented Feb 27, 2018

This solves date-fns bundling issue.

There is another issue related to date-fns.
au import date-fns doesn't work, the importer/strategies/amodro rejected it because of missing main in package.json.

I think it is quite safe to use "index.js" as the default value of package.main, because that's how nodejs deals with missing "main" anyway. @JeroenVinke I can make another PR if you agree to do so.

@JeroenVinke
Copy link
Collaborator

@huochunpeng thanks for the PR, i'll have a look

about using index.js as default, I think that should be fine since it's the last one, but I would prefer if we could check to see if that file exists before trying to execute the strategy

@3cp
Copy link
Member Author

3cp commented Feb 28, 2018

Sure, to ease my setup, I will create PR for that after this one is merged.

@3cp
Copy link
Member Author

3cp commented Mar 25, 2018

nodeIdCompat only fixes requrejs setup, not systemjs.

systemjs/systemjs#1807

@3cp 3cp changed the title fix(bundler): fix commonjs node id compatibility. WIP fix(bundler): fix commonjs node id compatibility. Mar 26, 2018
@3cp
Copy link
Member Author

3cp commented Mar 26, 2018

There is complication in test. All apps need to update aurelia-karma.js in order to run test normally.
Which means this is a breaking change.

Plus this doesn't help on systemjs setup.

I am working on a better solution.

@3cp
Copy link
Member Author

3cp commented Jul 25, 2018

obsoleted by #862

@3cp 3cp closed this Jul 25, 2018
@3cp 3cp deleted the fix-commonjs-node-id-compatibility branch October 9, 2018 04:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants